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.
1 participant
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.
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.
GET /lean/v0/node/identityreports the local ENR alongside the peer id,grouped into a
NodeIdentitystruct.quicport: one with only audpentryis kept as a discv5 seed even though it cannot be dialed over libp2p.
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 limitation: one lean devnet is not separated from another
Lean's
fork_digestis the hardcoded cross-client dummy0x12345678, so theeth2check separates lean from non-lean but not one lean devnet fromanother. 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.
Resolved: discovery is no longer one-sided
An earlier revision of this PR shipped with the ENR we reported and the ENR
we served being different records.
DiscoveryServer::spawntook an ethrexStoreand derived its own record from the localNode, with no way to seedthe consensus entries, so what it answered queries with carried
ip,udpand
secp256k1but none ofeth2,attnetsorquic. We found and admittedlean peers, but a lean peer applying these same rules to what ethrex served
would have refused us for a missing
quicentry.spawnnow takes a preparedNodeRecord, and we pass the onebuild_local_enrproduces, so the two are the same bytes. Discv5's IP voting
edits andre-signs that record rather than rebuilding it, so the consensus entries
survive a sequence bump. The empty in-memory
Storethat existed only tosatisfy the old signature is gone, along with the
ethrex-storagedependency.Testing
make lintclean.cargo test --workspace --profile release-fast --no-fail-fast: 601 passed,0 failed, including the forkchoice, signature, STF and SSZ spec tests.
oversized-
attnetsranking attack, subnet ranking, andspawn_discoverybinding a real socket (including
--discovery.advertise-ipand a busy port).Draft because the ethrex dependency is still an unmerged branch.