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.
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--discovery.portgives it its own UDP socket (it must differ from--gossipsub-port, and the node refuses to start otherwise rather thanfailing later with an opaque
EADDRINUSE). Static bootnode dialing isuntouched.
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. The same number sizes ethrex's peer table,
so one flag governs both ends. 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.
Dependency
ethrex-p2pis pinned to the unmergedfeat/discovery-peer-requirementsbranch, which carries the unified
DiscoveryServer, the peer table and thePeerFilterseam.Cargo.lockpins the exact commit (currentlybf401280),so builds are reproducible. This should be repointed at a main revision
before merge.
Note that ethrex still uses libssz 0.2.2 while ethlambda is on 0.3.0, so the
dependency graph now carries both. Nothing SSZ-typed crosses the boundary
(lean's
EnrForkIdis its own type), but it is worth knowing.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.
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.
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 target.Testing
make lintclean.cargo test --workspace --profile release-fast --no-fail-fast: 600 passed, 0 failed, 7 ignored,including the forkchoice, signature, STF and SSZ spec tests.
oversized-
attnetsranking attack, subnet ranking, subnet coverage countingonly connected peers, and
spawn_discoverybinding a real socket (including--discovery.advertise-ipand a busy port).Draft because the ethrex dependency is still an unmerged branch.