Repository navigation
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
🤖 Kimi Code ReviewThe PR improves separation of concerns and error handling in the discovery service. Here is the review:
General observations
Verdict: This is a clean refactor that improves error handling and separation of concerns. No changes required. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewNo findings. The diff looks sound. Moving ENR construction out of discovery and into Residual risk: there’s no added test coverage for the new startup error path or for preserving the startup ENR contents, so this is relying on existing discovery tests rather than a targeted regression test. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewI've reviewed the diff, the surrounding code in Review: PR 7147 — build the local ENR outside the discovery serverThis is a clean, mechanical refactor. The record-construction logic is moved verbatim from Correctness
Minor/nit observations (non-blocking)
Security / consensusNo consensus-relevant logic here — no opcode, gas, trie, or RLP changes. The ENR content and signing logic are untouched; only which layer constructs the record changed. No new attack surface introduced. OverallThe refactor does exactly what it claims: moves ENR construction to the caller and turns two panics into propagated errors, with no behavior change in the current single call path. Nothing blocking. The PR is honest about being a narrow prerequisite step (not fixing the Automated review by Claude (Anthropic) · sonnet · custom prompt |
Lines of code reportTotal lines added: Detailed view |
Integration commit, with no PR of its own: neither #7145 nor #7147 can make this change alone, because each removes one of the two things `storage` was for. #7145 moved the discv4 fork-id check behind `PeerFilter`, which left the `DiscoveryServer::store` field unused and removed it. #7147 moved the local ENR out of `spawn`, which was the last reader of the parameter. Together the parameter is dead, and `spawn` drops back under the argument-count lint, so the `#[expect]` #7147 needed goes with it. `DiscoveryServerError::Store` stays. Nothing in `discovery/` produced a `StoreError` even before either PR, so that variant is pre-existing dead code rather than something these changes stranded.
b44e5cc to
cf74eff
Compare
cf74eff to
06cd48b
Compare
| local_node: Node, | ||
| local_node_record: NodeRecord, |
There was a problem hiding this comment.
These should be deduped in the future
abb7120 to
55c3798
Compare
55c3798 to
802fc81
Compare
`DiscoveryServer::spawn` derived the record it advertises from `local_node`,
then stamped our own fork id onto it. Neither is discovery's decision to make.
What a node publishes about itself is the caller's: which entries the record
carries beyond the addresses, and what `seq` it resumes from. Deriving it inside
`spawn` left no way to say either, and forced a `panic` on a signing failure
that the caller returns as an error.
Take a `NodeRecord` instead of building one, and move the construction to
`start_network`, which already holds the node, the signer and the store. The
resulting record is byte-identical: `from_node` at `INITIAL_ENR_SEQ` followed by
`set_fork_id`, which bumps `seq` once, exactly as before.
That was the last reader of the `Store` `spawn` took. With the fork-id screening
moved behind `PeerFilter` in the previous commit, discovery no longer touches our
chain at all, so the parameter goes.
The two `expect`s become a `NetworkError`. `init_network` ends in
`.expect("Network starts")`, so an unsignable record still aborts startup rather
than leaving a node running without discovery; it now does so from one place and
with the underlying `NodeError` attached. A chain that cannot supply a fork id
still leaves the entry out and starts, since that record is usable and a peer
requiring `eth` simply passes us over.
This is a prerequisite for handing discovery the record `get_local_node_record`
already builds at startup. That one carries a `seq` persisted across restarts
and is the record `admin_nodeInfo` reports, while discovery gossips a separate
one starting from 1, so the two disagree today.
802fc81 to
11cc8bb
Compare
Resolutions, all in main's favor where main superseded the branch: - DiscoveryConfig: main dropped initial_lookup_interval (#7163); the branch's nat_extip_set field is added to main's struct and every construction site sets all three fields explicitly, so the branch's Default impl is gone too. - update_local_ip: main's edit-based version (#7143) wins. It preserves every ENR entry, which strictly subsumes the branch's rebuild that carried over only the fork id. - Discv5State keeps main's shape minus the IP-voting predictor this branch extracts into discovery/ip_predictor.rs. The five in-file voting tests go with the mechanism; their coverage lives on in test/tests/p2p/discovery/ip_predictor_tests.rs, and main's three update_local_ip ENR-preservation tests are kept. - DiscoveryServer drops the branch's store field: main moved ENR building out of discovery (#7147), so discovery no longer needs storage; apply_predicted_ip goes through main's update_local_ip. - start_network/DiscoveryServer::spawn keep main's signatures plus the branch's shared_local_node parameter. - The native-rollup RPC path (#6418, newer than this branch) is ported to the SharedLocalNode-taking init_rpc_api; no discovery runs there, so the shared identity is constant after construction.
…ambdaclass#6811) Follow-up to lambdaclass#6713, which deferred geth-style devp2p endpoint prediction. Closes lambdaclass#6714. On discv4-only devnets the majority-vote IP predictor never reached quorum because it only ran on discv5, so the node kept advertising its private/bridge IP and got 0 inbound peers. Extract `IpPredictor` out of `Discv5State` into `discovery/ip_predictor.rs`, owned by `DiscoveryServer` and fed by both discv4 (`PONG.to`, the peer's observed source of our PING) and discv5 PONGs. Applied via the existing `update_local_ip`, which bumps ENR seq and re-signs. `--nat.extip` sets `ip_override_locked`, preventing any predictor update; address-family mismatches are skipped. Votes discard only never-routable addresses (loopback/link-local/unspecified); RFC1918/unique-local private IPs are kept as candidates. On finalize the predictor prefers a public winner and falls back to a private one only if no public IP reaches quorum. A NAT/SNAT node converges on its public IP (peers observe the routable source); a node on a flat private network (e.g. a kurtosis enclave, where no public IP is ever observed) converges on its reachable private IP instead of advertising nothing. Tests from `discv5/server.rs` are relocated to `ip_predictor.rs`, plus discv4 PONG feeding, flat-network private convergence, and prefer-public-over-private. ## Post-restructure merge (2026-08-27) Main restructured discovery underneath this PR; the merge resolves in main's favor wherever main superseded it: - **`update_local_ip`**: main's edit-based version (lambdaclass#7143) wins — it preserves every ENR entry, strictly subsuming this branch's rebuild that carried over only the fork id. - **`DiscoveryServer` keeps no `store`**: main moved ENR building out of discovery (lambdaclass#7147), so the field this branch added for fork-id re-derivation is no longer needed; `apply_predicted_ip` goes through main's `update_local_ip`. - **`DiscoveryConfig`**: main dropped `initial_lookup_interval` (lambdaclass#7163); only `nat_extip_set` is added, and all construction sites set every field explicitly. - **The five in-file `Discv5State` voting tests** go with the mechanism they tested; their coverage lives on in `test/tests/p2p/discovery/ip_predictor_tests.rs` (8 tests), and main's three `update_local_ip` ENR-preservation tests are kept. - **The native-rollup RPC path** (lambdaclass#6418, newer than this branch) is ported to the `SharedLocalNode`-taking `init_rpc_api`. Post-merge verification: workspace clippy and `--features l2` clippy clean, fmt clean, 105 `ethrex-p2p` unit + 151 p2p integration + 57 rpc integration tests pass, and the vote wiring is confirmed intact (discv4 PONG → `record_ip_vote` → `apply_predicted_ip("discv4")`, discv5 likewise, prune tick → `check_timeout` → `apply_predicted_ip("timeout")`). ## Test plan - `cargo test -p ethrex-test ip_predictor` passes - discv4-only devnet (discv5 disabled): node converges to the correct external IP after 3 PONGs from distinct peers - `--nat.extip` still hard-locks the advertised address --------- Co-authored-by: ilitteri <ivanlitteri@hotmail.com> Co-authored-by: Ivan Litteri <67517699+ilitteri@users.noreply.github.com>
Motivation
DiscoveryServer::spawnderived the record it advertises fromlocal_node,then stamped our own fork id onto it. Neither is discovery's decision to make.
What a node publishes about itself belongs to the caller: which entries the
record carries beyond the addresses, and what
seqit resumes from. Deriving itinside
spawnleft no way to say either.It also forced a
panicon a failure the caller reports as an error, via twoexpects on the signing path.Description
spawntakes aNodeRecordinstead of building one, and the construction movesto
start_network, which already holds the node, the signer and the store.The resulting record is byte-identical to what
spawnproduced:from_nodeatINITIAL_ENR_SEQfollowed byset_fork_id, which bumpsseqonce. No entry,sequence number or signature changes.
That was the last reader of the
Storespawntook. #7145 moved the fork-idscreening behind
PeerFilterand removed theDiscoveryServer::storefield itfed; with the ENR built elsewhere, discovery no longer touches our chain at all,
so the parameter goes too. Neither change could drop it alone, which is why this
sits on top of #7145 rather than on
main.The two
expects become aNetworkError.init_networkends in.expect("Network starts"), so an unsignable record still aborts startup ratherthan leaving a node running without discovery; it now does so from one place and
with the underlying
NodeErrorattached. A chain that cannot supply a fork idstill leaves the entry out and starts, since the record is usable for discovery
and a peer that requires
ethsimply passes us over until we republish.What this unblocks
cmd/ethrexalready builds a local record, inget_local_node_record. That oneresumes from a
seqpersisted in the node config file (or a unix timestamp onfirst run), and is the record
admin_nodeInforeports and that shutdown writesback. Discovery never saw it, and gossiped its own starting from
INITIAL_ENR_SEQ, so the ENR peers receive and the ENR the node reports havedifferent sequence numbers, and only the gossiped one carries
eth.Restarting therefore re-announces at
seq2 no matter how high the previousrun climbed, and a peer holding a cached higher
seqtreats the new record asstale. Handing discovery the record that already exists fixes that, but it is a
behaviour change and wants its own PR; this one only makes it expressible.
Tests
No new ones: the record is built from the same two calls in the same order, so
there is no new behaviour to pin. Existing
p2punit and integration tests pass.Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.