feat(p2p): add opt-in discv5 peer discovery - #579
Conversation
Lean nodes could only meet through a static bootnode list, so every new node needed an operator to hand it peers. This wires ethrex's discv5 stack in behind `--discovery.enable`: the node builds and signs its own ENR, joins the DHT on its own UDP socket, and dials what it finds over libp2p QUIC. Static bootnode dialing is untouched and discovery is off by default, so nothing changes for an operator who does not ask for it. Admission follows the beacon phase0 p2p spec, mirroring lighthouse's `eth2_fork_predicate`: the `eth2` fork digest must match, a differing `next_fork_version`/`next_fork_epoch` is explicitly tolerated, and the peer must advertise a `quic` port. The checks live in a `LeanFilter` that ethrex's peer table runs as each ENR arrives, so a record is judged where it lands rather than at dial time, and is judged afresh whenever the peer publishes a higher-`seq` record. Survivors are ranked by how many attestation subnets they cover that no connected peer does, so discovery fills subnet gaps first. A peer's `attnets` is self-reported, so subnet ids at or beyond the local committee count are dropped before ranking sees them. `ethrex-p2p` is pinned to the unmerged `feat/discovery-peer-requirements` branch, which carries the unified `DiscoveryServer`, the peer table, and the `PeerFilter` seam. Repoint it at a main revision once that merges. Known gap: `DiscoveryServer::spawn` builds its own local record and offers no way to seed the consensus entries, so the ENR ethrex answers queries with carries `ip`/`udp`/`secp256k1` but not `eth2`, `attnets` or `quic`. Discovery is one-sided until `spawn` can take a prepared record: we find and admit lean peers, but a lean peer applying these same rules to what ethrex serves would refuse us. See `docs/discovery.md`.
## 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.
Quality pass over the discovery feature. No behaviour change; the one observable difference is that a malformed bootnode ENR now warns once instead of twice, because the file is parsed once. Reuse and layering: - Merge `ethlambda-types::enr` into `p2p::discovery::enr`. The shared types crate grew an SSZ container and a `libssz_derive` use for a single consumer crate, and split `encode_attnets` from the `ATTNETS_ENR_KEY` that gives it meaning. `FORK_DIGEST` stays in `types::constants`, where a second crate does use it. - Move the dial loop out of `lib.rs` into `discovery::dial`, matching how `gossipsub::handler` and `req_resp::handlers` already keep their bodies out of the shared actor file. `DiscoveryState`, `covered_subnets` and `local_peer_id` go with it, so dial policy is editable without touching shared actor state. - Add `P2PServer::forget_discovered_peer` so the two teardown paths (`ConnectionClosed` and `OutgoingConnectionError`) share a seam instead of both reaching into `peer_attnets`. - One `quic_multiaddr()` for the two dial paths that were building the same `ip / udp / quic-v1 / p2p` chain, and `ethrex_p2p::utils::public_key_from_signing_key` in place of the hand-rolled uncompressed-SEC1 conversion (which had three copies). - Fold the `!= 0` filter into `read_quic_port` and have `parse_enr` call it. The two spellings of "no dialable quic port" had already drifted. - Drop `read_extra`: ethrex's `pairs.extra()` already returns `Bytes`, so the wrapper only added a copy on a path that runs per arriving ENR. Simplification: - `subnets_from_attnets(bits, committee_count)` replaces decode-everything-then-clamp. Iterating our own committee makes the clamp unforgettable rather than documented in three places, and stops a padded hostile bitfield allocating ~18 KB before being discarded. - `DiscoveryError` via `thiserror` replaces 12 hand-rolled `String` errors; p2p was the only crate in the workspace without it. `main.rs` loses its `map_err(|err| eyre::eyre!(err))` bridge. - Delete `DiscoveredPeer::label` and `DiscoveryHandle::bound_addr`: both were read only by tests, and `label` allocated a base58 string on every admission while the one log line uses `%peer_id`. The ENR-vs-bound-port test now asserts on the record's `udp` entry, which is the invariant. - Delete `RejectReason::as_str`, whose five strings restated the five variant docs for one `debug!`. - Read the bootnode file once (`#[derive(Clone)] Bootnode`) and inline the locals copied out of `options.discovery`. Move the unspecified-IP warning into `spawn_discovery`, next to the code that picks the value. Dependencies: - `DiscoverySpawnConfig::node_key` takes `Vec<u8>` like its sibling `SwarmConfig::node_key`, which removes the binary's direct `secp256k1` dependency and its version-coupling to ethrex's workspace. - p2p: drop the unused `recovery` feature, move `bytes` and `rand` to dev-dependencies (both are test-only). Revert `pub mod req_resp` / `pub mod encoding` to private: they were widened for an `examples/mainnet_gossip.rs` that is not in the tree, and making `req_resp` public also exposed the actor-facing `handlers` module. `NodeIdentity` reaches the identity route behind an `Arc`, so a polled endpoint stops cloning two startup-fixed strings per request.
The merge with main placed it after a blank line, outside the list it belongs to and flush against the new Development heading.
Our lock pinned 669de531, which is no longer on the branch: it was rebased away, so the build only kept working because the old commit was still in the local Cargo cache. A fresh clone would not have resolved it. Three API changes come with f30b16d5: - `PeerTableServer::spawn_with_filter` takes `impl PeerFilter + 'static` instead of `Box<dyn PeerFilter>`, so the call site drops its `Box::new`. - `NodeRecordPairs::set_extra_int` takes a `u64` rather than any `RLPEncode`, which is deliberate upstream: a generic bound under a method named for integers would re-open the encode-a-`Vec<u8>`-as-a-list footgun that `set_extra` exists to close. - Both setters now answer whether the entry was stored, `false` for a key the record already has a typed field for. `attnets`, `eth2` and `quic` are all outside that dictionary and the tests assert each one lands in the built record, so `local_pairs` does not check the answers. `PeerFilter::accepts` is unchanged, so `LeanFilter` needed no edit.
The helpers predate `f7fddb9dc` upstream, which added the `set_extra*` accessors so callers stop writing `extra_fields` directly. They still assigned the whole bag and hand-rolled the RLP for each entry, which made these tests the one place an ENR was assembled differently from the way `build_local_enr` assembles one: a `pair()` returning `(Bytes, Bytes)`, `Bytes::from(..).encode_to_vec()` per payload, and a comment explaining which of the two encodings that produced. `record_with` now takes a closure over `NodeRecordPairs` and the entries go through `set_extra`/`set_extra_int`, so a record these tests accept is one built the way production builds it, encoding included. Assertions are unchanged. Since nothing names `Bytes` any more, the `bytes` dev-dependency and the `ethrex_rlp::encode::RLPEncode` import go with it. `set_extra_encoded` stays unused: it exists for values the typed setters cannot express, such as a deliberately malformed RLP list, and no test wants one yet.
Bumps ethrex to the feat/discovery-peer-requirements tip (f30b16d5 -> bf401280, rebased onto main 24.0.0), which reworks `DiscoveryServer::spawn` to take a prepared `NodeRecord` instead of a `Store` it derived one from. That closes the gap docs/discovery.md called "the record ethrex serves is not the record we report": ethrex built its own copy from the local `Node`, so what it answered discv5 queries with carried `ip`, `udp` and `secp256k1` but none of `eth2`, `attnets` or `quic`. A lean peer applying our own admission rules to that record rejected us for the missing `quic` entry, so discovery found peers but could not be found by them. We now hand `spawn` the same record `enr_url` reports, and ethrex edits and re-signs it on IP voting rather than rebuilding, so the consensus entries survive a sequence bump. The empty in-memory ethrex `Store` existed only to satisfy the old signature, so both it and the `ethrex-storage` dependency go, along with the `DiscoveryError::Store` variant that could no longer be constructed.
Exposing the record over `/lean/v0/node/identity` is a separate decision from discovery itself, and it reads better once the P2P actor owns the record rather than the binary passing it along. Restores `start_rpc_server` to the plain peer-id string it took before this branch; the ENR is still logged at startup.
The binary had to know discv5's startup sequence: await `spawn_discovery`, handle its failure, and thread the resulting handle into the actor that polls its peer table. `P2P::spawn` now takes the config and does that itself, so discovery's lifetime starts with the actor that consumes it and the binary is left with the CLI-to-config translation. Discovery still starts before the swarm adapter, so a fatal failure such as a busy UDP port surfaces before any actor is running.
The dial cutoff and the discv5 peer table were sized by one hardcoded constant, so a node could not be told to hold more peers than the author picked. Both now read `--discovery.target-peers`, defaulted to 200: high enough that a node keeps filling subnet coverage rather than stopping at the first handful of peers it happens to meet. A target of 0 is accepted and means "discover and serve, never dial".
The admission filter and the bootnode parser each matched on a Result only to log the error arm and rebuild the shape the combinators already give: `inspect_err` plus `is_ok`/`ok` says it directly. Discovery startup is the same idea over an Option: `OptionFuture` awaits the spawn only when there is one, so `?` still carries a bind failure out without a `None` arm written by hand. Scoped to the code this branch already touches; no behaviour change.
`for_test` sat in the production half of the file behind its own `#[cfg(test)]`, away from the helpers it belongs with. Moving the impl into `mod tests` puts it next to `raw_record` and the record builders, and the module's own `cfg` covers it.
| /// | ||
| /// Discovery is started before the swarm adapter so a fatal discovery | ||
| /// failure (a busy UDP port, say) surfaces before any actor is running. | ||
| pub async fn spawn( |
There was a problem hiding this comment.
The async here is only needed in the discv4 path of ethrex's discovery server, and it can probably be refactored to not need it. I leave it like this for now since it's not really a problem for the integration, but it's something to keep in mind for the future.
| store: Store, | ||
| node_names: HashMap<PeerId, String>, | ||
| discovery: Option<DiscoverySpawnConfig>, | ||
| ) -> Result<P2P, DiscoveryError> { |
There was a problem hiding this comment.
This Result can probably be removed too.
| // rather than sharing one: the two carry the same fork id and committee | ||
| // count, which is what makes their judgments agree. | ||
| let filter = LeanFilter::new(EnrForkId::local(), config.attestation_committee_count); | ||
| let peer_table = PeerTableServer::spawn_with_filter( |
There was a problem hiding this comment.
ethrex paces its lookups by peers.len() / target_peers, but peers is only populated by NewConnectedPeer, which arrives from ethrex's RLPx layer. We never send it, so the ratio is pinned at 0 and lookups already run at the fast 500ms end.
peer_table.new_connected_peer takes a PeerConnection, so fixing this would require changes from ethrex's side. Let's revisit this after this PR is merged
🤖 Codex Code Review
No consensus-layer logic, attestation validation, STF, XMSS, or SSZ code paths were changed here; the review surface is networking/discovery only. I couldn’t run the targeted tests in this sandbox because the pinned Rust toolchain could not be downloaded offline. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewOverall Assessment: This is a well-structured, security-conscious implementation of discv5 peer discovery. The code correctly handles ENR encoding/decoding, fork ID validation, and subnet-based peer ranking. No critical vulnerabilities found. Detailed Feedback1. Security & Correctness
pub(crate) fn subnets_from_attnets(bits: &[u8], committee_count: u64) -> Vec<u64> {
(0..committee_count) // Bounds iteration to local config, not peer's bitfield length
.filter(|subnet| ...)
}This prevents the ranking algorithm from being dominated by fabricated subnet claims (validated in tests at line 408).
pub(crate) fn read_quic_port(record: &NodeRecord) -> Option<u16> {
record.pairs().extra_int::<u16>(QUIC_ENR_KEY).filter(|port| *port != 0)
}
2. Error Handling & Robustness
.filter_map(|enr_str| {
parse_enr(&enr_str)
.inspect_err(|reason| warn!(...))
.ok()
})This prevents a single bad bootnode entry from crashing the node.
3. Architecture & Performance
4. Consensus & Networking Safety
5. Minor Improvements
6. TestingThe test coverage is comprehensive:
Acknowledgments
Conclusion: LGTM. The implementation correctly handles the consensus-layer discovery requirements while maintaining robustness against malformed or hostile ENRs. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewReview: PR 579 —
|
`admit` and `parse_enr` each re-derived the IPv4-over-IPv6 preference and the secp256k1-to-libp2p key decode. Both answer "who does this record belong to, and where do we reach them", so letting the two drift would mean the bootnode parser and the admission filter disagreeing about the same ENR. They now share `read_ip`/`read_public_key`, next to the `read_quic_port` they already shared. Three smaller things in the same pass, none of them behaviour changes: `forget_discovered_peer` was the only inherent `P2PServer` method defined outside `lib.rs`; every other submodule reaches the actor through a free function taking `&mut P2PServer`, so `grep 'impl P2PServer'` no longer missed part of its mutating surface. The dial loop cloned the peer-table ref and the filter on every tick but only used them when refilling an empty candidate queue, so the clones now happen under that condition. Discovery items nothing outside the crate names drop to `pub(crate)`. Only `DiscoverySpawnConfig`, `DiscoveryError` and `DEFAULT_DISCOVERY_TARGET_PEERS` cross into `bin/ethlambda`; the rest read as API with no consumer.
…th the rest "If lean ever meets a real network" and the two `tcp` notes described a future whose shape is not settled: what to do about a live fork schedule, and the interop cost of publishing no `tcp` entry. Neither is something an operator reading this page acts on today, and both would need rewriting rather than updating once lean's fork story lands. `lean_discovered_peers_dialed_total` moves to `docs/metrics.md`, where every other metric is already documented in table form. It goes under the custom (non-leanMetrics) heading, since that table's Supported column tracks spec conformance and discv5 discovery is ours alone.
|
Read the full diff, then pulled ethrex at the pinned commit 1.
|
--discovery.target-peers |
completion | lookup interval |
|---|---|---|
| 200 (default) | 0/200 = 0.0 |
500ms, forever (never eases to the 10s steady state) |
| 0 (documented mode) | 0/0 = NaN |
0ns |
- Default: discv5 iterative lookups stay pinned at the startup rate for the life of the process — roughly 20x the intended steady-state FindNode traffic, permanently.
--discovery.target-peers 0:NaNsurvives the easing curve, andNaN as u64saturates to 0, sosend_after(Duration::ZERO, ..., LookupV5)re-fires immediately. That's an unthrottled lookup loop spinning the actor and flooding FindNode packets. The CLI help advertises this exact value as supported: "0 means discover and serve, never dial."
Two things to fix: guard target_peers == 0 (in validate_discovery, or clamp in spawn_discovery), and decide what the flag actually means for the peer table — either feed connection state back into it, or stop passing target_peers there and document the flag as dial-loop-only. Right now docs/discovery.md's "The same number sizes the discv5 peer table" is true but functionally inert, which is the more misleading of the two.
2. The PR description promises an RPC change that isn't in the diff
GET /lean/v0/node/identityreports the local ENR alongside the peer id, grouped into aNodeIdentitystruct.
crates/net/rpc/ isn't among the changed files, and main's IdentityResponse carries only version and peer_id. There is no NodeIdentity struct anywhere. Correspondingly, DiscoveryHandle::local_enr is built, logged once, and then dropped — DiscoveryState::new doesn't keep it, so in production it's write-only (its own doc comment says "a future RPC identity endpoint", which is the honest version). Either drop the bullet or land the endpoint.
3. The dependency bump is much larger than "add a discovery dep"
ethrex-p2p goes 8.0.0 -> 24.0.0, dragging 65 ethrex lock entries and 891 deletions of unrelated lockfile churn. It also lands a duplicate libssz (0.2.2 alongside 0.3.0) in the graph. The PR body notes the libssz split and that nothing SSZ-typed crosses the boundary — that's correct, EnrForkId is lean's own type — but the review surface here is a whole ethrex upgrade riding along with the feature, not just the discovery seam.
4. Minor
read_quic_port's doc comment is wrong about why it works. It claims "an absent entry RLP-decodes to0u16via left-padding." It doesn't — ethrex'sextra_intreturnsNoneon the.find(...)?before any decode happens (types.rs:452-455). The.filter(|port| *port != 0)is still needed for an explicitquic: 0; only the stated reason is off.forget_discovered_peercan fire for a still-connected peer. It's called unconditionally onOutgoingConnectionError(lib.rs:726). If a second dial to an already-connected discovery peer fails — e.g. via the bootnode redial path — itsattnetsare dropped while the peer is live, socovered_subnetsunder-counts. Only affects ranking eagerness, never correctness. Gating on!server.connected_peers.contains(&pid)would close it.
Already flagged above, worth keeping
udp: 0is accepted verbatim as a discv5 seed — real, and it's the exact asymmetry the code deliberately avoids forquic. Worth fixing for consistency alone.- Bootnode parsing degrading to warnings means a fully-malformed bootnode file boots an isolated node silently. Also note
build_swarmskips quic-less bootnodes atdebug!level, so feeding a beacon-chain list yields zero static dials with nothing atwarn!. branch = "..."instead ofrev = ..., while the PR is out of draft. The body still says "Draft because the ethrex dependency is still an unmerged branch," but the PR is no longer a draft. This is the merge blocker.
What's solid
Checked the claims that mattered and they hold. update_local_ip really does edit+re-sign rather than rebuild, preserving eth2/attnets/quic across an IP-voting seq bump — and ethrex has tests for exactly that (update_local_ip_preserves_entries_it_does_not_touch). The serve/report ENR unification is genuinely closed: one NodeRecord feeds both enr_url() and spawn. subnets_from_attnets iterating the local committee rather than the peer's bitfield is the right defense and is tested with a 290-byte hostile pad. The set_extra vs. bare-Vec<u8> footgun the comments warn about is real (types.rs:465) and correctly avoided. Test coverage on admission, ENR round-trip, and bootnode parsing is genuinely thorough, and docs/discovery.md is unusually good — including honest limitations.
Finding 1 is the only one I'd call blocking on its own merits, alongside the dependency pin.
feat/discovery-peer-requirements is still unmerged and still moving, so tracking it by branch means a force-push or a new commit silently changes what this crate builds against: Cargo.lock records the drift, but only after someone runs `cargo update`, and nothing in the manifest says which commit was reviewed. Pinning the rev makes that an explicit edit. The lockfile churn is only the source strings; the resolved commit is unchanged.
The flag was also handed to ethrex's peer table, which reads it as the denominator of the discv5 lookup pacing: `peers.len() / target_peers`. That table only counts peers registered through `NewConnectedPeer`, which carries an RLPx `PeerConnection`; we connect over libp2p and register nothing, so the numerator is permanently 0 and the flag could never mean what it said there. At the default it merely pinned lookups to the startup rate. At the documented `--discovery.target-peers 0`, "discover and serve, never dial", it was worse: `0/0` is NaN, NaN survives the easing curve, and `NaN as u64` saturates to 0, so the lookup timer re-fired with no delay at all: an unthrottled FindNode loop from a value the CLI advertises as supported. The table now gets a named constant whose only job is to not be zero, with the reasoning attached, and the flag governs the dial loop alone. The pacing limitation that remains is documented rather than implied away: closing it needs ethrex to learn about non-RLPx connections.
`read_quic_port` already rejects an explicit `quic: 0`, on the grounds that it names no listener, but `udp` was read straight off the record. A `udp: 0` entry would then be seeded into discv5's routing table as a contact on a port nothing is bound to. Same reasoning, same treatment: the record survives as a static dial target if it has a usable `quic` entry, and is dropped entirely when neither port is dialable. Also corrects `read_quic_port`'s stated reason for returning `None` on a missing entry. It claimed an absent entry "RLP-decodes to `0u16` via left-padding"; ethrex's `extra_int` looks the key up and returns `None` before it decodes anything, so the port-zero filter is what handles the explicit case and nothing else.
`forget_discovered_peer` ran unconditionally on `OutgoingConnectionError`, which is also where a *failed second dial to an already-connected peer* lands: the bootnode redial path can produce exactly that. The peer stays connected, but its `attnets` are dropped, so `covered_subnets` stops crediting it and the ranking treats subnets we already cover as gaps. Gate the cleanup on the peer actually being gone. The dial-never- established case, which is what the call is there for, is unaffected.
Both ways a bootnode file can come to nothing were silent above `debug`: - Every entry unusable. Each rejection warns individually, but the empty list that results is indistinguishable from having configured no bootnodes at all. - Every entry `quic`-less. Each skip is logged at `debug`, which is right for one record, but a beacon-chain bootstrap list is entirely `tcp`/`udp` records: feeding one in produces zero static dials with nothing above `debug` to say so. Either case boots a node that peers with nobody unless discovery is enabled, so each now gets one line at `warn` with the count.
The discovery work this crate builds on has landed on ethrex's main, so there is no longer a branch to track: feat/discovery-peer-requirements' unified `DiscoveryServer`, `PeerTableServer::spawn_with_filter` and the `PeerFilter` seam are all in `275da92`, along with the discv5 tests that back the claims in docs/discovery.md. Still pinned by `rev` rather than following main: ethrex publishes no release to name, and a moving pin would change what this builds against with nothing in the manifest saying so. Two consequences worth naming: - `DiscoveryConfig` lost its `Default` impl and its unused `initial_lookup_interval` field, so both protocol flags are now named explicitly. That is the better spelling anyway: `..Default::default()` was quietly relying on a default `discv4_enabled` we always override. - ethrex is on libssz 0.3.0 as of this commit, the version ethlambda uses, so the dependency graph no longer carries two libssz majors.
ethrex now publishes releases, so the pin can name one instead of a bare `main` commit. v25.0.0 is a descendant of the previous rev and the diff between them touches nothing this crate builds against, so the discovery module, peer table, and `PeerFilter` seam are unchanged.
pablodeymo
left a comment
pablodeymo
left a comment
There was a problem hiding this comment.
CI is green (Lint, Test 602 passed, Build, License) and the branch is MERGEABLE. This is genuinely strong work — the docs and the comments carry real reasoning, and the test coverage on the adversarial paths is better than most PRs get. One thing I'd want fixed before we rely on it, one description fix, and a handful of nits.
I checked the two headline claims against the actual ethrex v25.0.0 source rather than taking the description's word:
- ENR extras survive IP voting ✅ —
NodeRecord::edit(types.rs:598) clones pairs, applies the closure, bumpsseq, re-signs;update_local_iptouches onlyip/ip6. The "we serve the record we report" fix is real. - ENRs are signature-verified before reaching
LeanFilter✅ —peer_table.rs:1525anddiscv5_handlers.rs:202. Soattnets/eth2can only ever be self-attested, never forged under another node's key, which is exactly the threat the oversized-attnetsclamp is scoped to. - Also ✅: libp2p identity and the discv5 node id derive from the same secp256k1 key (
lib.rs:277vsdiscovery/mod.rs:126), so the self-dial guard atdial.rs:90actually works. Anddraw_candidatesis right thatpasses_filter == Nonecontacts are still offered (peer_table.rs:1332), and correctly drops them viacontact.record.as_ref().
1. admit() does no sanity check on the advertised IP — and our own default publishes 0.0.0.0
discovery/admission.rs:152 takes whatever read_ip returns and feeds it straight into quic_multiaddr. There is no filter for unspecified, loopback, link-local, multicast or broadcast addresses. I checked, and ethrex does not filter either: is_private_ip appears only in the IP-vote accept path (discv5/server.rs:148) and in WHOAREYOU rate limiting (discv5_handlers.rs:766), never against discovered records. So LeanFilter is the only gate, and it does not gate this.
This is not hypothetical, because of how the defaults compose:
main.rs:131hardcodes the bind IP to0.0.0.0discovery/mod.rs:137:advertise_ip.unwrap_or(config.bind_ip)- so a node with
--discovery.enableand no--discovery.advertise-ippublishes a signed, admissible ENR advertising0.0.0.0
Every peer that receives it builds /ip4/0.0.0.0/udp/<quic>/quic-v1/p2p/<id> and dials it. On Linux that connects to localhost, so each reader dials its own QUIC listener, fails the peer-id check, and repeats. The warn! at discovery/mod.rs:198 correctly tells the publisher, but the cost lands on every reader, who gets no warning at all.
Two fixes, and I'd take both:
- Cheap, closes the common case: make
--discovery.advertise-iprequired when--discovery.enableis set, invalidate_discoveryalongside the existing port check. Five lines, and it makes the footgun unreachable. - The actual hardening: a
RejectReason::UnroutableAddressinadmit(). Careful with a blanketis_global()check —docs/discovery.md:36recommends127.0.0.1for local devnets, so that would break the documented setup. Suggestion: reject unspecified/multicast/broadcast unconditionally, and allow loopback/private only when our own advertised IP is in the same class.
The feature is off by default, so this is "fix before anyone turns it on" rather than a hard blocker on the merge itself. Fine by me either here or as an immediate follow-up.
2. The description is stale on the dependency
The whole Dependency section argues for a decision that was then reversed. It says rev 275da92 (24.0.0), "still pinned by rev", "ethrex publishes no release to name", "Still a rev and not a moving pin". The branch head is 7751fb5 chore(deps): pin ethrex to the v25.0.0 release tag → tag = "v25.0.0" (f3b90bc). Same for the last bullet under "Changes since the first revision".
The rest of the numbers hold up, so it is just that section: the lockfile delta is 486/783 against the body's "~485 / ~780" ✅, and the crate delta is exactly as described ✅.
Worth adding to the description, because it pre-empts the obvious objection: the heavy ethrex crates were already in the graph on main — ethrex-levm, ethrex-vm, ethrex-blockchain, ethrex-storage and ethrex-trie all predate this PR. This adds ethrex-guest-program + ethrex-l2-common and drops ethrex-threadpool. The net dependency count actually goes down, 722 → 712. Saying that stops anyone relitigating "why is an EVM in our consensus client".
(The Test job at 20m40s vs ~10min on main is almost certainly the cold cache from the changed pin rather than real cost, but a warm-cache re-run would settle it.)
3. peer_attnets and the dial counter drift when swarm.dial fails synchronously
dial.rs:100-104 inserts into peer_attnets and increments lean_discovered_peers_dialed_total before the dial. But swarm_adapter.rs:150-154 swallows a synchronous swarm.dial error at debug!, and libp2p returns Err synchronously — with no OutgoingConnectionError event — for DialError::LocalPeerId, NoAddresses, DialPeerConditionFalse (already connected or already dialing) and Denied. In those cases forget_discovered_peer never runs.
Not a correctness bug — covered_subnets filters on connected_peers, so a stale entry contributes nothing to ranking — but the map is unbounded over the process lifetime and the counter overcounts. DialPeerConditionFalse is the realistic trigger: a discovery dial racing an in-flight bootnode redial to the same peer.
Cheapest fix is moving the insert and the metric into the ConnectionEstablished handler, where you know the dial took.
4. --discovery.port defaults to 9000, same as --gossipsub-port, so --discovery.enable alone always fails
cli.rs:140, with validate_discovery at cli.rs:166. The error message is good, but a default that is never valid is a strange default: --discovery.enable on its own is a guaranteed startup failure. 9001, or gossipsub_port + 1, would make the flag work standalone. Pre-existing from the earlier CLI PR, but this is the PR that gives the flag meaning, so it seems like the right place to fix it.
5-8. Nits
- Ranking goes stale mid-batch.
dial_tickranks once at refill (dial.rs:84-86), then drains one per tick over up to 8 ticks / 40s. By candidate #8 the coverage snapshot is 40s old, which undercuts the stated "fill coverage gaps first" goal. Recomputingcoveredand picking the max on each pop is O(8). - Skipped candidates are dropped for good.
dial.rs:89-94continues an already-connected candidate out of the queue, and ethrex has already put it inalready_tried_peers(peer_table.rs:1338), which only clears when a full pool scan finds nothing. A peer that disconnects later is not re-offered promptly. Self-heals in a small devnet, slower in a big pool — probably just wants a comment. DiscoveryHandle::local_enris dead outside tests.spawn_discoverylogs it atdiscovery/mod.rs:197, thenDiscoveryState::newdrops it. The doc calls it "what a future RPC identity endpoint would read", but that endpoint was explicitly reverted in7a02f094. Either drop the field, or drop the speculative sentence.fork_digest()parses a&strat runtime and panics (enr.rs:65-69). Cosmetic —FORK_DIGESTalso builds every gossip topic, so a malformed one fails elsewhere first — but it could beconst.
One observation rather than a finding: --discovery.target-peers defaults to 200 at one dial per 5s, which is ~17 minutes to saturate. Fine for realistic pool sizes, but the default reads more ambitious than the dial loop can deliver.
`peer_attnets` and `lean_discovered_peers_dialed_total` were written before `swarm.dial`, but libp2p rejects some dials synchronously — `LocalPeerId`, `NoAddresses`, `Denied`, and `DialPeerConditionFalse` (already connected, or already dialing) — and those raise no `OutgoingConnectionError`, so nothing ever ran `forget_discovered_peer` or corrected the counter. The realistic trigger is a discovery dial racing an in-flight bootnode redial to the same peer. Ranking was never wrong, since `covered_subnets` filters on `connected_peers`, but the map grew unbounded over the process lifetime and the counter overcounted. `SwarmHandle::dial_accepted` reports back whether the swarm queued the dial; the insert and the metric now happen only then.
…d alone `--discovery.port` and `--gossipsub-port` both defaulted to 9000 and both bind UDP, so `--discovery.enable` on its own was a guaranteed startup failure: a default that is never valid. discv5 keeps 9000 and libp2p QUIC moves to 9001, one apart, so the flag works standalone. The collision check stays for the case where an operator sets the ports explicitly. Every devnet script already passes `--gossipsub-port`, so nothing in tree changes behaviour.
Merging main brought #579's cli.rs tests, which call `CliOptions::parse_from`. `CliOptions` has been a `clap::Args` group since `node` became a sub-command, so that constructor no longer exists and the test build stopped compiling. Git merged both sides cleanly — the conflict is semantic, so nothing flagged it. The tests now go through `command::parse_node_options`, a single test-only helper that runs the real dispatch and hands back the node options. The command.rs tests use it too, so there is one way to parse a node command line in tests rather than two.
2 participants
What
Adds opt-in discv5 peer discovery, so a lean node can find peers instead of
being handed them. Off by default;
--discovery.enableturns it on and needsnothing else: discv5 keeps port
9000and libp2p QUIC now defaults to9001,so the two UDP sockets no longer collide. Overriding either onto the other is
still rejected at startup rather than failing later with an opaque
EADDRINUSE. Static bootnode dialing is untouched.Built on ethrex's discovery stack:
DiscoveryServerruns discv5-only andwrites what it finds into a
PeerTable, whichP2PServerpolls, filters anddials over libp2p QUIC. We build the local ENR ourselves and hand it to
spawn, so the record ethrex answers queries with is the one this nodereports.
How peers are judged
Admission follows the beacon phase0 p2p spec, mirroring lighthouse's
eth2_fork_predicate:eth2entryfork_digestnext_fork_version/next_fork_epochquicportsecp256k1,ip/ip6These live in a
LeanFilterhanded to the peer table as itsPeerFilter, soeach record is judged the moment it arrives rather than at dial time. No
rejection is final: the peer table re-runs the filter as soon as the peer
publishes a higher-
seqENR, so a node that adds aquicentry or gains anaddress through discv5's IP voting is reconsidered without a restart.
Admitted peers are ranked by how many attestation subnets they advertise that
no connected peer covers, so discovery fills coverage gaps first.
attnetsisself-reported and unauthenticated, so subnet ids at or beyond the local
committee count are dropped before ranking sees them: otherwise an ENR padding
its bitfield with a few hundred bytes of
0xFFwould outrank every honest peerforever.
Dialing stops once
--discovery.target-peerspeers are connected and resumeswhen the count drops back below it. That is the whole of what the flag does: it
is the dial loop's cutoff, and it is deliberately not handed to ethrex's peer
table (see the limitation on lookup pacing below). A target of
0means"discover and serve, never dial".
Also here
--discovery.advertise-ipseparates the bound address from the advertisedone, for a node behind NAT or on a host whose public IP is not what it binds.
quicport: one with only audpentryis kept as a discv5 seed even though it cannot be dialed over libp2p.
lean_discovered_peers_dialed_totalcounts dials discovery initiated, asdistinct from the static bootnode dials every node makes. Connection outcomes
are not duplicated: a discovery dial that succeeds or fails already shows up
in
lean_peer_connection_events_total.docs/discovery.mdcovers the ENR layout, the admission rules, the operatorflags and the known limitations.
On the dependency
The heavy ethrex crates were already in the graph on
main:ethrex-levm,ethrex-vm,ethrex-blockchain,ethrex-storageandethrex-trieallpredate this PR, which already depended on
ethrex-p2pbyrev. What thischanges is the pin, from that
revto thev25.0.0release tag. The delta isethrex-guest-programandethrex-l2-commonin,ethrex-threadpoolout, andthe workspace's total crate count goes down: 722 to 712 unique names in
Cargo.lock.Known limitations
One lean devnet is not separated from another. Lean's
fork_digestis thehardcoded cross-client dummy
0x12345678, so theeth2check separates leanfrom non-lean but not one lean devnet from another. Two devnets running
this code will peer with each other. Closing that needs lean to adopt a
genesis-derived fork digest, which is a cross-client change to gossip topic
names.
discv5 lookups run at the startup rate. ethrex paces its iterative lookups
by how full its own peer table is, easing from one every 500ms to one every 10s
as the table fills. That table only counts peers registered through
NewConnectedPeer, which carries an RLPx connection: ethlambda connects overlibp2p and registers nothing, so the count is pinned at zero and the pacing
never eases. Lookups keep firing every 500ms for the life of the process,
roughly 20x the intended steady-state
FindNodetraffic. This is also why--discovery.target-peersis not passed to that table: a target of0wouldmake the pacing divide by zero, and
NaN as u64saturates to a zero-delayre-fire. Closing it properly means ethrex learning about non-RLPx connections,
which is an upstream change.
attnetsis not a fixed-width SSZBitvector. The spec's isBitvector[ATTESTATION_SUBNET_COUNT], a constant every conformant clientshares, which is what makes an undelimited bitfield decodable. ethlambda
derives the width from
attestation_committee_count, which is runtimeconfiguration, so two nodes can legitimately exchange bitfields of different
lengths. The bit-packing convention is the spec's; only the width is
negotiable. Readers tolerate a foreign length by treating bits past the end as
unset, and a peer's advertised subnets are clamped to the local committee count
before they influence anything.
Changes since the first revision
DiscoveryServer::spawnused to take an ethrexStoreand derive its ownrecord, so what it answered queries with carried
ip/udp/secp256k1butnone of
eth2,attnetsorquic: a lean peer applying these same rules toit would have refused us.
spawnnow takes a preparedNodeRecordand wepass what
build_local_enrproduces. IP votingedits and re-signs thatrecord rather than rebuilding it, so the consensus entries survive a sequence
bump. The empty in-memory
Storeand theethrex-storagedependency aregone.
GET /lean/v0/node/identity. It islogged once at startup instead; the RPC crate is untouched by this PR.
udp: 0bootnode entry is now read as no entry at all, matching whatquic: 0already did, so discv5 is never seeded with a contact on a portnothing listens on.
warn. Boththe every-entry-unusable case and the every-entry-
quic-less case (abeacon-chain bootstrap list) previously left the node isolated with nothing
above
debug.second dial to an already-connected peer used to clear its
attnets, makingsubnet-coverage ranking under-count.
P2P::spawn, which takes anOption<DiscoverySpawnConfig>and owns the resulting handle, rather thanmainspawning the server and threading a handle in.--discovery.target-peersreplaces the hardcoded dial cutoff, and isdeliberately not handed to ethrex's peer table: see the lookup-pacing
limitation above for what that would have done at
0.v25.0.0release tag. The firstrevision tracked the unmerged
feat/discovery-peer-requirementsbybranch,which let a force-push change what this built against. That work has since
landed and shipped, so the pin names a release rather than a moving branch or
a bare commit.
--discovery.enableworks on its own.--gossipsub-portnow defaults to9001, one above--discovery.port. Both are UDP and both defaulted to9000, so enabling discovery without also moving a port was a guaranteedstartup failure: a default that is never valid. The collision check stays for
the case where an operator sets them explicitly. Every devnet script passes
--gossipsub-portalready, so nothing in tree changes behaviour.peer_attnetsandlean_discovered_peers_dialed_totalwere written beforeswarm.dial, but libp2p rejects some dials synchronously —LocalPeerId,NoAddresses,Denied, andDialPeerConditionFalse(already connected, oralready dialing) — with no
OutgoingConnectionErrorto follow, so nothingever cleared the entry or corrected the counter. The realistic trigger is a
discovery dial racing an in-flight bootnode redial to the same peer. Ranking
was never wrong (
covered_subnetsfilters onconnected_peers), but the mapgrew unbounded over the process lifetime and the counter overcounted.
SwarmHandle::dial_acceptednow reports back whether the swarm queued thedial, and the insert and the metric happen only then.
Testing
make lintclean.make test: 604 passed, 0 failed, 7 ignored, including the forkchoice,signature, STF and SSZ spec tests.
--discovery.enablestand alone, and thecollision check still fires when both are pointed at one port.
oversized-
attnetsranking attack, subnet ranking, subnet coverage countingonly connected peers, both zero-port bootnode cases (
udp: 0read as absent,and a record dropped when neither port is dialable), and
spawn_discoverybinding a real socket (including
--discovery.advertise-ipand a busy port).