[MegaRedHand]

🗒️ Description / Motivation

make run-devnet on this branch does not finalize. The lean-quickstart devnet runs every node with --network host and passes no --discovery.port, and discv5 is currently always on for both chains. So every node tries to bind the default UDP 9000. The first node gets it, and the others exit at startup:

Error: failed to start discv5 discovery
   0: failed to bind discovery socket on 0.0.0.0:9000: Address already in use (os error 98)

With 3 nodes, only ethlambda_0 survived. It holds one validator of three, so it saw only its own blocks (the head moved every third slot) and justified_slot stayed at 0. The aggregator was one of the nodes that died.

Only beacon needs discovery on unconditionally, because published mainnet bootnode ENRs carry no quic entry and cannot be dialed statically. A lean network has no other discv5 speakers, so a crawl there finds only other ethlambda nodes. This PR gives node back the --discovery.enable switch that main still has, off by default.

What Changed

Area	Change
bin/ethlambda/src/cli.rs	New lean-only --discovery.enable flag (LeanOptions::discovery_enable, default false). Network::discovery_enabled() is the single source of truth: the flag on lean, always true on beacon. Port validation moves to Options::validate_ports(), which applies the discovery rules only when discovery runs
bin/ethlambda/src/main.rs	Builds DiscoverySpawnConfig only when discovery is enabled. Rewords the missing --bootnodes warning to mention the flag
crates/net/p2p/src/lib.rs	P2P::spawn takes Option<DiscoverySpawnConfig> and P2PServer.discovery is an Option<DiscoveryState>, the same shape as main. None binds no socket and never schedules the first DiscoverPeers tick
crates/net/p2p/src/discovery/dial.rs	Dial-loop helpers take target_peers as an argument instead of reading it from the discovery state. The refill step is extracted into refill_candidates so the borrow of the optional state does not span the dial's .await. forget_discovered_peer still drops custody when discovery is off
docs/, CLAUDE.md, Dockerfile	"Always on" becomes "always on for beacon, opt-in for node"

Correctness / Behavior Guarantees

• beacon is unchanged. It always runs discovery and runs the same port checks. It now rejects --discovery.enable with a usage error instead of accepting a flag it would ignore.
• node without --discovery.enable binds no discovery socket, publishes no ENR and never runs the dial loop. It peers from --bootnodes alone, as node does on main.
• Without discovery, --discovery.port is not checked against --gossipsub-port, and --gossipsub-port 0 is accepted, since no ENR names it. The TCP clash check against --api-port/--metrics-port applies either way. It now skips 0 explicitly, because a 0 gossip port can reach it.
• --discovery.enable has the same name and semantics as on main, so launch commands carry over between the two branches.

Tests Added / Run

The port tests in cli.rs used to check that discovery was always on for node. The ones that test discovery-on rules now pass --discovery.enable, and these tests are new:

• discovery_is_opt_in_on_node_and_always_on_on_beacon
• beacon_has_no_discovery_enable_flag
• without_discovery_the_discovery_port_is_not_checked
• gossipsub_port_zero_is_accepted_without_discovery
• port_zero_never_counts_as_a_clash, which now covers both settings

Commands run:

• make fmt, make lint
• cargo test --profile release-fast -p ethlambda --bins: 223 passed
• cargo test --profile release-fast -p ethlambda-p2p --lib: 207 passed

This has not been run on a local devnet yet.

✅ Verification Checklist

• Ran make fmt: clean
• Ran make lint (clippy with -D warnings): clean
• Ran make test: only the unit tests of the two touched crates were run (see above)

[github-actions]

🤖 Claude Code Review

Review: PR 637, make discv5 opt-in on node again

I read the diff only and did not build it or run the tests. It looks correct and self-consistent, and I found no blocking issues.

What it does: discv5 is always on for beacon and gated behind --discovery.enable on node. It revives the flag that an earlier commit removed.

What looks good

• Network::discovery_enabled() is the single source of truth. Options::validate_ports and run_node both use it, so the port rules and the spawn decision can't disagree.
• Making DiscoverySpawnConfig an Option and server.discovery an Option<DiscoveryState> is a sound way to express "off".
• The first DiscoverPeers tick is only scheduled when discovery is on. The handler also returns early without rescheduling if discovery is None, so the dial loop can't start by accident.
• dial_tick takes target_peers as an argument, which removes the server.discovery.target_peers reads that would otherwise panic or need unwraps.
• In refill_candidates, the mutable borrow of discovery ends before undersupplied_custody_columns(server) runs, and the code re-borrows afterwards. That is correct, though the double as_mut() is a little awkward.
• Validation is scoped correctly. With discovery off, the --discovery.port clash check and the --gossipsub-port 0 ban are skipped, because no UDP socket is bound and no ENR is published.
• Adding gossipsub_port != 0 && to the TCP clash check is right. Two zeros (OS-picked ports) aren't a clash.
• Test coverage is thorough. It covers:
  • the default for each chain;
  • beacon rejecting --discovery.enable as an unknown argument;
  • the discovery-off port cases;
  • the port-0 cases;
  • the TCP-versus-HTTP clash under both discovery modes.
• The docs, the Dockerfile comment and the no---bootnodes warning text are updated to match.

Minor nits, non-blocking

1. peer_attnets is now only maintained when discovery is on. That is fine, but any future reader of covered_subnets or the attnet-based ranking outside the discovery path must handle None. The if let Some guards in the disconnect and dial paths already do this.
2. With discovery off, a node with no --bootnodes only warns. That is reasonable, since it can still be dialed, and the warning text now says so.
3. CLAUDE.md still says discv5 is "always on" and that there is no --discovery.enable. The PR description and this diff update the docs in docs/ but, as far as I can tell, not that paragraph, so it should be brought in line.
4. crates/net/p2p/src/req_resp/handlers.rs and another test module now build Some(DiscoveryState::new(..)). A small test helper would cut that repetition.

Overall, this is a clean, well-tested revert of the always-on behaviour, scoped to lean only.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which makes discv5 peer discovery opt-in for node (lean) chains while keeping it always-on for beacon (mainnet). Let me analyze the changes across all files.

Summary

The PR is well-structured and addresses a real operational need: co-located devnet nodes binding the same default UDP port. The core change is sound—beacon always needs discovery (mainnet bootnodes aren't statically dialable), while node can rely on --bootnodes alone.

However, I've identified several issues ranging from minor code quality concerns to potential correctness problems.

---

Issues Found

1. Redundant Option unwrapping in refill_candidates — crates/net/p2p/src/discovery/dial.rs:273-276

if let Some(discovery) = server.discovery.as_mut() {
    discovery.candidates.extend(admitted);
}

Problem: At line 256, discovery is already bound via let Some(discovery) = server.discovery.as_mut(). The subsequent if let Some(discovery) = ... at line 273-276 is redundant—the discovery variable is already in scope and is the same Option. This suggests a refactoring artifact where the original inline code was extracted without cleaning up.

Fix: Use the already-bound discovery directly:

// Line 276: change from
if let Some(discovery) = server.discovery.as_mut() {
    discovery.candidates.extend(admitted);
}
// to simply:
discovery.candidates.extend(admitted);

---

2. forget_discovered_peer inconsistent Option handling — crates/net/p2p/src/discovery/dial.rs:84-89

pub(crate) fn forget_discovered_peer(server: &mut P2PServer, peer_id: &PeerId) {
    if let Some(discovery) = server.discovery.as_mut() {
        discovery.peer_attnets.remove(peer_id);
    }
    // ... custody removal follows unconditionally

Problem: The custody comment says "custody still goes" without discovery, but the actual custody removal code is not shown in the diff. If custody removal depends on peer_attnets or other discovery state, this could be inconsistent. More importantly, this function is called from connection teardown paths—if discovery is off, peer_attnets was never populated, so the remove is a no-op, which is correct.

However: The comment at line 82-83 says "Without discovery there are no attnets to drop, but custody still goes." This implies custody should be handled unconditionally, but I cannot verify the custody code is actually present below this snippet. Please confirm custody removal follows unconditionally after this block.

---

3. Potential panic in dial_progress with target_peers == 0 — crates/net/p2p/src/discovery/dial.rs:114-118

let total = if target_peers == 0 {
    1.0
} else {
    server.connected_peers.len() as f64 / target_peers as f64
};

Problem: The target_peers == 0 guard is correct, but consider: when would target_peers be 0? DiscoveryState::target_peers comes from DiscoveryConfig::target_peers which defaults to 200. A user would need to explicitly set --discovery.target-peers 0.

Question: Is target_peers == 0 a valid configuration? If discovery is enabled with 0 target peers, the dial loop would spin at MAX_DIAL_RATE_PER_SECOND (see dial_interval at 0.0 progress = 0ms interval... actually let me check).

Looking at dial_interval (line ~178):

pub(crate) fn dial_interval(progress: f64) -> Duration {
    // ...
    if progress <= 0.0 { return Duration::from_millis(0); } // or similar?
}

Actually the diff doesn't show dial_interval, but if progress returns 1.0 when target_peers == 0 (due to total = 1.0), then dial_interval(1.0) would be slow. But outbound_ratio with reservation = Some(0) returns 1.0, so progress = 1.0. This seems intentional—0 target peers means "don't dial"—but it's subtle. Consider documenting this edge case or rejecting target_peers == 0 at config validation.

---

4. dial_tick returns false on None discovery after refill_candidates — crates/net/p2p/src/discovery/dial.rs:252-255

refill_candidates(server);
let Some(discovery) = server.discovery.as_mut() else {
    return false;
};

Problem: refill_candidates early-returns if server.discovery is None. So this let Some(discovery) check is technically unreachable after refill_candidates returns normally. However, this is defensive and harmless—keep it for clarity, but consider a debug_assert! or comment noting the invariant.

Actually, looking more carefully: refill_candidates does let Some(discovery) = server.discovery.as_mut() else { return; }; and returns (). So after it returns, server.discovery is guaranteed Some. The subsequent let Some(discovery) is indeed redundant but safe.

Suggestion: Add a comment like // refill_candidates guarantees discovery is Some or use .expect() to document the invariant.

---

5. Test helper node_ports shadows parse behavior — bin/ethlambda/src/cli.rs:632-636

fn node_ports(extra: &[&str]) -> eyre::Result<()> {
    Options::from(parse(extra)).validate_ports()
}

Problem: This helper calls Options::from(parse(extra)), but parse returns NodeOptions, and Options::from(NodeOptions) was defined at lines 175-186. The From impl sets network: Network::Lean(options.into()). This is correct but indirect.

Minor issue: The helper name node_ports is slightly misleading—it validates ports for node subcommand args. But beacon_ports exists too. This is fine, just noting the asymmetry: node_ports takes &[&str], beacon_ports takes Vec<&str>.

---

6. Missing test: node with --discovery.enable and --gossipsub-port 0 — bin/ethlambda/src/cli.rs

The test gossipsub_port_zero_is_rejected_while_discovery_runs at line 748 tests node_ports(&["--discovery.enable", "--gossipsub-port", "0"]), good.

But there's no test for node without --discovery.enable and --gossipsub-port 0 combined with --discovery.port explicitly set. The port validation skips discovery checks when discovery_enabled is false, but --discovery.port is still parsed into common.discovery.port. If a user sets --discovery.port 9000 --gossipsub-port 9000 without --discovery.enable, it should pass (and does, per without_discovery_the_discovery_port_is_not_checked).

However: What if they later enable discovery via admin RPC or config reload? The port would collide. This is a design question, not a bug—documented behavior is that --discovery.* flags are ignored without --discovery.enable.

---

7. P2PServer::discovery field documentation — crates/net/p2p/src/lib.rs:1175-1178

/// The dial loop's state. `None` when discv5 is off, which only a lean
/// node started without `--discovery.enable` is.
pub(crate) discovery: Option<DiscoveryState>,

Minor: "which only a lean node started without --discovery.enable is" is grammatically awkward. Suggest: "None when discv5 is off; this only happens for a lean node started without --discovery.enable."

---

8. Inconsistent discovery field type in test support — crates/net/p2p/src/lib.rs:2511-2514

discovery: Some(crate::discovery::dial::DiscoveryState::new(
    discovery,
    built.local_peer_id,
)),

Observation: The test support code always wraps in Some(...). This is correct for tests that expect discovery, but there's no test path for None. Consider adding a test helper for discovery-disabled scenarios, or document that tests assume discovery is on.

---

9. spawn method signature change is API-breaking — crates/net/p2p/src/lib.rs:1036

pub async fn spawn(
    // ...
    discovery: Option<DiscoverySpawnConfig>,
    // ...
)

Note: This is a public API change. The PR updates all call sites, but external users (if any) would break. Since this is a binary crate's internal API, this is acceptable, but worth noting for versioning.

---

10. Potential race in P2P::spawn discovery scheduling — crates/net/p2p/src/lib.rs:1105-1115

let discovery_enabled = server.discovery.is_some();
// ...
if discovery_enabled {
    send_after(
        DIAL_INTERVAL_AT_ZERO_PEERS,
        handle.context(),
        p2p_protocol::DiscoverPeers,
    );
}

Question: Is there any path where server.discovery could be modified between the discovery_enabled check and the send_after? No—this is synchronous construction before the actor runs. The code is correct.

However, consider: what if a future change adds dynamic discovery enablement? The current architecture would require restarting P2P. This is fine for the stated use case.

---

11. CLAUDE.md and docs updates are comprehensive — No issues

The documentation updates in CLAUDE.md, docs/cli.md, docs/discovery.md, and docs/metrics.md are thorough and consistent with the code changes. The Dockerfile comment update is correct.

---

12. main.rs module reordering — bin/ethlambda/src/main.rs:1-5

-mod beacon;
 mod banner;
+mod beacon;
 mod benchmark;

Observation: This is a pure style change (alphabetical ordering?). Harmless but unrelated to the PR's purpose. Consider keeping in a separate commit for cleaner history.

---

Security Considerations

1. ENR with port 0: When discovery is on, --gossipsub-port 0 is rejected because the ENR would advertise an undialable port. This is correct and important for network health—nodes publishing useless ENRs waste peer table space.

2. Bootnode-only peering: A lean node without --discovery.enable relies entirely on --bootnodes. The warning message at default_bootnodes (line 895) is updated correctly:

   "only find peers via discv5 (--discovery.enable) or by being dialed."

   This accurately reflects that inbound connections are still possible.

3. No new attack surface: Making discovery opt-in doesn't introduce new vulnerabilities. The discv5 protocol handling is unchanged; only the scheduling of the dial loop is conditional.

---

Performance Implications

1. Reduced resource usage for devnet nodes: Not binding a UDP socket and not running the dial loop saves CPU and memory. This is the PR's main benefit.

2. No regression for beacon nodes: The Option<DiscoveryState> indirection is minimal; Option is optimized well in Rust.

---

Correctness Verdict

The core logic is correct. The main issues are:

• Item 1 (redundant Option unwrap in refill_candidates) — should be fixed before merge
• Item 4 (defensive but redundant check in dial_tick) — minor, add comment or simplify
• Item 7 (awkward documentation) — polish

The test coverage is thorough for the port validation logic, with good edge cases around 0 ports and discovery-disabled scenarios.

---

Recommended Changes

// crates/net/p2p/src/discovery/dial.rs:273-276
-    if let Some(discovery) = server.discovery.as_mut() {
-        discovery.candidates.extend(admitted);
-    }
+    discovery.candidates.extend(admitted);

And at line 252-255, either:

refill_candidates(server);
// Invariant: refill_candidates returns only when discovery is Some
let discovery = server.discovery.as_mut().expect("discovery enabled by refill_candidates");

Or keep the let Some with a comment explaining it's defensive.

---

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

[github-actions]

🤖 Codex Code Review

Overall this looks solid. The PR cleanly makes discovery optional for node while preserving beacon behavior, and the Option<DiscoverySpawnConfig> / Option<DiscoveryState> split is consistent through startup and the dial loop.

• No consensus-layer correctness issues stood out in the touched code: this PR is confined to CLI/network startup and discovery pacing, not fork choice, attestation validation, finalization, state transition, XMSS, or SSZ.
• I did not spot any obvious memory-safety or security bugs in the Rust changes. The discovery state is now guarded consistently before use in bin/ethlambda/src/main.rs:675, crates/net/p2p/src/lib.rs:1040, and crates/net/p2p/src/lib.rs:1392.

One thing I’d still flag:

• Potential config mismatch on lean nodes with discovery disabled: build_swarm still receives target_peers: common.discovery.target_peers unconditionally in bin/ethlambda/src/main.rs:691. If lean-mode swarm admission/connection limits are derived from this value anywhere down-stack, a user who disables discovery may still unexpectedly cap peers based on a discovery-specific knob. The comments imply lean uses unlimited connections, so this may be fine, but it’s worth double-checking that target_peers is ignored for lean when discovery is off. If not, I’d suggest threading an explicit “discovery disabled”/“unlimited” signal rather than always passing common.discovery.target_peers.

Nice touch:

• The Options::validate_ports() change is an improvement: it moves the discovery-port checks behind the actual chain/runtime decision in bin/ethlambda/src/cli.rs:170 and avoids false rejections for node without --discovery.enable.
• The dial-loop pacing change in crates/net/p2p/src/lib.rs:1407 and crates/net/p2p/src/discovery/dial.rs:243 also looks like a good performance fix: backing off to DIAL_INTERVAL_AT_TARGET when no dial was opened should avoid useless tight polling on small devnets.

I couldn’t run the targeted tests because cargo/rustup tried to write under /home/runner/.rustup and hit a read-only filesystem. If you want, I can still do a second pass focused just on the build_swarm connection-limit path to confirm the potential mismatch above.

---

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

[pablodeymo]

LGTM. Beacon behavior is unchanged, the flag matches main's, and the dial loop really never runs without it. A few non-blocking nits:

• dial_tick gets target_peers from the caller and then unwraps server.discovery again; the if let Some blocks at dial.rs:318 and dial.rs:353 can't fail. Reading target_peers inside dial_tick, or an expect with the reason, would make the invariant explicit.
• Maybe a warn! when --discovery.* flags are passed to node without --discovery.enable, since they're silently ignored.
• The mod banner/mod beacon reorder also fixes the base's fmt failure; worth a line in the description.
• Could you confirm a 3-node make run-devnet finalizes before merging? That was the motivating failure.