fix(l1): back off failed dials, pace discovery by saturation and add --p2p.netrestrict

[ilitteri]

Motivation

An operator running several ethrex containers in a private devnet behind a carrier-grade NAT reported that each container opens ~10 TCP connections/s and ~8 UDP packets/s towards addresses outside the devnet, enough for their ISP to rate-limit them, while the other clients in the same devnet stay quiet. They asked for a way to keep ethrex from crawling anything outside the devnet.

The numbers are ethrex's own startup pacing, and ethrex never leaves startup pacing in a small network:

• Both the RLPx dialer (100 ms → 600 ms) and the discovery lookup tick (500 ms → 10 s, × alpha 3, per protocol) ease only on connected / --p2p.target-peers. With the default target of 100 a devnet stays at the fastest rate forever.
• Dial candidates come from a flat pool with no per-node state: a failed dial is not recorded, nothing ever leaves the pool except by FIFO eviction at 10k entries, and the dialer ignores contacts discovery already marked unreachable. With k candidates that never answer, the node keeps sending about 10·k/(k+1) SYN/s.
• Every lookup in a saturated network converges immediately and the next one starts on the next tick with a fresh random target, so the FindNode stream never stops.
• There is no way to confine traffic to an address range, and turning both discovery protocols off also drops the bootnodes, so there is no static-peers mode either.
• On the discv4 side an ENR was only requested for contacts that already had one, so most discv4 contacts never got a fork-id verdict (discv5 already treated "no record" as seq 0).

Description

• Per-node dial backoff. Pool entries remember their last dial and consecutive failures. A candidate is not offered again for 35s · 2^(failures-1), capped at 30 minutes (35 s is geth's dialHistoryExpiration). A successful connection resets the counter. Contacts discovery could not reach (disposable) are skipped by the dialer and evicted from the pool on prune. The per-sweep already_tried_peers set is gone.
• Saturation-aware lookup pacing. The peer table exposes a monotonic discovered_count. A lookup that finishes without adding anything to the pool counts as empty, and each empty lookup in a row doubles the wait before the next one (500 ms → 1 s → 2 s → … → 10 s). In-flight lookups keep the fast tick so they still finish promptly.
• The RLPx initiator idles at the steady interval (600 ms) when it has nothing to dial instead of polling the table ten times a second.
• --p2p.netrestrict <CIDR,...> (env ETHREX_P2P_NETRESTRICT), mirroring geth's --netrestrict: nodes discovered outside the list are never stored, pinged or dialed; bootnodes outside are dropped with a warning; inbound TCP and UDP from outside are discarded before any handshake.
• Static-peers mode. With --p2p.discv4=false --p2p.discv5=false the bootnodes are still handed to the dialer, so the node connects only to them.
• discv4 ENR gap. The pong and ping handlers treat a missing record as seq 0 and request the ENR, as the discv5 handler already did.

Deliberately left out: dialing only ENR-verified discv4 candidates, the way geth's AsyncFilter(RequestENR) does. That changes mainnet bootstrap throughput and should be measured on a real node first; the ENR-gap fix above moves verdicts closer without gating dials on them.

Docs: docs/l1/running/startup.md gains a "Private networks and devnets" section and docs/CLI.md is regenerated.

How to test: cargo test -p ethrex-p2p --lib covers the backoff schedule and eligibility, pool eviction of unreachable contacts, netrestrict filtering over both discovery paths, discovered_count, the saturation pacing curve and CIDR matching. To see it live, start two or three ethrex nodes on a local genesis with each other as --bootnodes and watch connection attempts with admin_peers or tcpdump: retries of an unreachable candidate now back off per node instead of recurring every pool sweep, lookups settle at one every 10 s, and with --p2p.netrestrict <devnet CIDR> anything outside is ignored (visible as Ignoring node outside --p2p.netrestrict at trace level and Dropping inbound connection from outside --p2p.netrestrict at debug level).

Checklist

• Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync. (Not needed: no Store changes.)

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

rpc-compat log-bearing cases excluded

Where: KNOWN_EXCLUDED_TESTS in .github/scripts/check-hive-results.sh counts out
eight hive rpc-compat cases — the four eth_getLogs cases, eth_getBlockReceipts/get-block-receipts-latest,
and three eth_getTransactionReceipt cases. They are exactly the cases whose recorded
response contains at least one log object; every case with an empty log array still runs.
Note this leaves eth_getLogs with no rpc-compat coverage at all, since all four of its
cases are in the set.

Why: ethrex populates blockTimestamp on log objects, as geth, besu, nethermind, reth
and erigon all do. hive's rpc-compat compares responses byte-exactly (jsondiff.FullMatch;
the lenient checkJSONStructure path applies only to cases upstream marks speconly), and
the corpus is pinned to execution-apis d08382ae (2025-02-10), whose recordings predate the
field — it entered the schema in execution-apis#639 and the fixtures in #846 (2026-07-22).
So the extra key cannot match, and this is a property of the pin rather than of the response.

The pin cannot move, and this is not temporary. The pin sits one commit before
execution-apis#627, which moved the test chain to a pre-merge genesis: the current corpus has
~36 proof-of-work blocks before its terminal total difficulty. ethrex does not support
pre-merge chains and will not, so importing that chain.rlp fails at block 1 —
validate_block_header has no pre-London base-fee path. Every revision carrying
blockTimestamp in its fixtures also carries that chain, so there is no revision that
satisfies both. Nor can the corpus be patched locally: rpc-compat's Dockerfile clones
ethereum/execution-apis by hard-coded URL, so the branch buildarg cannot point at a fork.

Coverage: the field itself is pinned by
block_timestamp_is_on_the_log_and_not_on_the_receipt in
crates/networking/rpc/types/receipt.rs, which asserts it is present on each log and absent
from the receipt level.

Removal: delete the entries if ethrex ever gains pre-merge chain import, or if upstream
marks these cases speconly so they are type-checked instead of compared byte-for-byte.

---

The stateless schema id does not identify the encoding

Where: STATELESS_INPUT_SCHEMA_ID in crates/common/types/stateless_ssz.rs.

Upstream keeps the stateless input schema id at 0x1501
(fork_index 0x15 << 8 | revision 0x01) across incompatible body changes. Three
encodings have now shipped under it: tests-zkevm@v0.6.2, then #3248 + #3278,
then #3356, which moved state, codes and public_keys from SszList to
ProgressiveList. ethrex speaks the last one.

The consequence is that the 2-byte prefix cannot be used to detect a stale or
mismatched bundle. A wrong-dialect input is accepted by the id check and then
fails later — in SSZ decode, or on a root that does not match — rather than being
rejected up front for what it is. only_amsterdam_schema_id_decodes therefore
proves less than its name suggests.

Worth raising upstream: a revision field that does not move across a body change
provides no version negotiation at all.

---

ZisK guest program hash changes with the unsync_cell gate

Where: crates/common/types/block.rs, transaction.rs.

The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.

The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.

This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rs → unsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.

---

Release signing key is an unprotected repository secret

Where: .github/workflows/tag_release.yaml.

MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.

This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:

1. Move MINISIGN_SECRET_KEY / MINISIGN_PASSWORD into a GitHub Environment with required
   reviewers, and add environment: to the two jobs that sign.
2. Add a ruleset targeting refs/tags/v* restricting who may create release tags.

Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds --p2p.netrestrict functionality and several P2P improvements. Let me analyze the changes systematically.

Overview

This PR introduces:

1. --p2p.netrestrict CLI flag to restrict P2P traffic to specific IP networks (CIDR notation)
2. Dial backoff mechanism to avoid hammering unreachable nodes
3. Discovery lookup saturation backoff when no new peers are found
4. Improved ENR sequence handling for discv4
5. Static peers mode when discovery is disabled

---

Critical Issues

1. Race condition in discovered_count tracking (peer_table.rs:604-605)

self.connection_pool.insert(node_id, PoolEntry::new(node));
self.discovered_count += 1;

The discovered_count increments even when the insert replaces an existing entry. IndexMap::insert returns the old value if the key existed, but it's ignored. This means discovered_count overcounts on duplicates, making the saturation detection in discovery less accurate.

Fix: Only increment when insert returns None (new entry):

if self.connection_pool.insert(node_id, PoolEntry::new(node)).is_none() {
    self.discovered_count += 1;
}

Wait — re-reading the test at line 2305, this appears intentional: "discovered_count_only_grows_on_nodes_new_to_the_pool". But insert_to_connection_pool doesn't check for existing entries before incrementing. Let me re-check...

Actually, looking more carefully: IndexMap::insert replaces the value and returns the old one. The test passes because do_new_contacts calls insert_if_new which checks !self.has_contact(&node_id) first. But new_contacts (plural) and new_contact_records also call insert_to_connection_pool directly. The discovered_count will overcount when the same node is rediscovered via different paths.

Severity: Medium — causes unnecessary discovery lookups when network is actually saturated.

---

Security Issues

2. UDP packet filtering happens after is_discv4_packet check (server.rs:386-393)

async fn route_packet(&mut self, data: &[u8], from: SocketAddr) {
    if !self.config.netrestrict.allows(from.ip()) {
        trace!(%from, "Dropping UDP packet from outside --p2p.netrestrict");
        return;
    }
    if is_discv4_packet(data) {
        // ...
    }
}

Actually, this looks correct — the netrestrict check happens first. ✓

But wait: is_discv4_packet only checks packet size minimum (98 bytes). A malicious actor could still send large UDP packets to trigger decoding work. However, this is pre-existing and not introduced by this PR.

3. No logging for dropped inbound TCP connections (network.rs:224-228)

if !context.netrestrict.allows(peer_addr.ip()) {
    tracing::debug!(peer = %peer_addr, "Dropping inbound connection from outside --p2p.netrestrict");
    continue;
}

This uses debug! level. For security-relevant drops (potential network partitioning attacks), warn! would be more appropriate, especially since this is a configuration meant to protect private networks. Operators should know if something is trying to reach their nodes.

---

Correctness Issues

4. ENR request logic in discv4_handle_ping uses wrong validation check (discv4_handlers.rs:377-385)

if let Some(contact) = self.peer_table.get_contact(node_id).await?
    && contact.was_validated()
    && let Some(received) = ping_message.enr_seq
    && received > contact.record.as_ref().map_or(0, |r| r.seq)
{
    self.discv4_send_enr_request(&node).await?;
}

The comment says "A contact we hold no record for counts as seq 0", but was_validated() returns true for contacts that passed PING-PONG. However, a bootnode might be validated but have no ENR yet. The old code checked stored_enr_seq separately; the new code conflates "has been validated" with "has ENR".

Actually, re-reading: was_validated() likely means "completed handshake", not "has ENR". The logic seems correct — we only request ENR from nodes we've validated, and we treat missing ENR as seq 0. ✓

But wait: what if was_validated() is false? Then we skip ENR request entirely for unvalidated contacts. The old code would still compare sequences. Is this intentional?

Looking at the comment: "Otherwise a discv4 node's ENR... would only ever be fetched by the slow random ENR lookup." This suggests the change is to fetch ENRs more aggressively from validated nodes. Seems reasonable.

5. Missing Default implementation for Discv4State fields (discv4/server.rs)

The new fields lookup_started_at_count and empty_lookups_in_a_row aren't shown with Default impl changes. Let me check if Discv4State derives Default...

Looking at the diff, Discv4State doesn't derive Default in the shown code. Discv5State has an explicit Default impl that initializes the new fields. If Discv4State derives Default, the new fields would be zero-initialized, which is correct. But if not, this could be uninitialized.

Actually, checking: Discv4State is shown without Default impl in the diff. The discv5/server.rs shows explicit Default with lookup_started_at_count: 0, empty_lookups_in_a_row: 0. The discv4/server.rs doesn't show a Default impl at all. This is inconsistent and potentially buggy if Discv4State is constructed elsewhere without these fields.

Fix: Ensure Discv4State has explicit Default or verify it derives it.

---

Performance Issues

6. Repeated NetRestrict::new(opts.netrestrict.clone()) calls (initializers.rs)

let netrestrict = NetRestrict::new(opts.netrestrict.clone());  // line 470
// ...
NetRestrict::new(opts.netrestrict.clone()),  // line 939
// ...
NetRestrict::new(opts.netrestrict.clone()),  // line 957

Three clones of the same Vec<IpNet>. Since NetRestrict uses Arc<[IpNet]>, the clones are cheap after the first, but the Vec clone is unnecessary. Better to create once and clone the NetRestrict (which is cheap due to Arc).

Fix: Create NetRestrict once in init_l1, then .clone() it (clones the Arc).

7. peer_table.discovered_count().await? called twice per lookup cycle (discv4_handlers.rs:132-141, discv5_handlers.rs:299-308)

let had_active = self.discv4.as_ref().is_some_and(|s| !s.active_lookups.is_empty());
// ... retain ...
let just_finished = had_active && self.discv4.as_ref().is_some_and(|s| s.active_lookups.is_empty());

// then later:
let count = self.peer_table.discovered_count().await?;

This is fine — the count is needed at different times. But note that discovered_count is a simple field read wrapped in an actor request; the async overhead seems unnecessary for a u64. Pre-existing pattern though.

---

Rust Best Practices

8. dial_backoff could use saturating_pow or checked math more cleanly (peer_table.rs:329-337)

fn dial_backoff(failures: u32) -> Duration {
    if failures == 0 {
        return Duration::ZERO;
    }
    DIAL_BACKOFF_BASE
        .checked_mul(1u32.checked_shl(failures - 1).unwrap_or(u32::MAX))
        .unwrap_or(DIAL_BACKOFF_MAX)
        .min(DIAL_BACKOFF_MAX)
}

The 1u32.checked_shl(failures - 1).unwrap_or(u32::MAX) is clever but opaque. For failures > 31, this returns u32::MAX, causing checked_mul to fail and fall back to DIAL_BACKOFF_MAX. This works but is hard to follow.

Suggestion: More explicit version:

fn dial_backoff(failures: u32) -> Duration {
    if failures == 0 {
        return Duration::ZERO;
    }
    let shift = (failures - 1).min(31); // u32::BITS - 1
    DIAL_BACKOFF_BASE.saturating_mul(1 << shift).min(DIAL_BACKOFF_MAX)
}

Actually, Duration::saturating_mul doesn't exist until Rust 1.66+. The current code is fine, just add a comment explaining the u32::MAX fallback.

9. PoolEntry::record_dial pessimistically counts as failure (peer_table.rs:350-353)

fn record_dial(&mut self, now: Instant) {
    self.last_dial_attempt = Some(now);
    self.dial_failures = self.dial_failures.saturating_add(1);
}

This increments dial_failures before knowing if the dial succeeds. The success path calls record_connected which resets to 0. This is correct but the naming is confusing — dial_failures includes the in-progress attempt.

Suggestion: Rename to consecutive_dial_attempts or add comment: "Incremented optimistically; reset on successful connection."

10. Inconsistent Display for NetRestrict (netrestrict.rs:45-55)

impl fmt::Display for NetRestrict {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        if self.0.is_empty() {
            return f.write_str("unrestricted");
        }
        for (i, net) in self.0.iter().enumerate() {
            if i > 0 {
                f.write_str(",")?;  // no space after comma
            }
            write!(f, "{net}")?;
        }
        Ok(())
    }
}

The Display output "10.0.0.0/8,172.16.0.0/12" doesn't match the CLI help's example "10.0.0.0/8,172.16.0.0/12" — actually it does match. But the log output at initializers.rs:472 uses %netrestrict which will call Display. Fine.

---

Memory Safety / Error Handling

11. unwrap_or(0) for ENR seq could panic on future u64 overflow? (discv4_handlers.rs:382)

contact.record.as_ref().map_or(0, |r| r.seq)

r.seq is u64. map_or(0, ...) is safe. No issue.

12. next_lookup_interval floating point concerns (discovery/mod.rs:70-85)

let backoff_ms = (lower_limit * 2f64.powi(empty_lookups_in_a_row.min(16) as i32)).min(upper_limit);
paced.max(Duration::from_micros((1000f64 * backoff_ms).round() as u64))

For empty_lookups_in_a_row = 16, 2f64.powi(16) = 65536. With lower_limit = 500.0, that's 32,768,000 which gets clamped to upper_limit (e.g., 10_000.0). The min(16) prevents infinity.

But: 1000f64 * backoff_ms for large values before min could be large, but min applies first. Actually no — min(upper_limit) is applied to backoff_ms, then multiplied by 1000. If upper_limit = 10_000.0, result is 10_000_000.0 microseconds = 10 seconds. Correct.

Wait, the units are confusing. lower_limit and upper_limit are in milliseconds (per lookup_interval_function docs). So backoff_ms is in milliseconds. Then 1000f64 * backoff_ms converts to microseconds. The function name lookup_interval_function returns Duration. Let me check...

Actually lookup_interval_function returns (u64, Duration) where the u64 is... unclear. Looking at usage in server.rs:

let interval = self.get_lookup_interval(DiscoveryProtocol::Discv4).await;
send_after(interval, ctx.clone(), ...);

And get_lookup_interval returns Duration. The lookup_interval_function returns Duration directly. Looking at the original code:

pub fn lookup_interval_function(progress: f64, lower_limit: f64, upper_limit: f64) -> Duration {
    // ...
    (1000f64 * (ease_in_out_cubic * (upper_limit - lower_limit) + lower_limit)).round() as u64,
)

Wait, that's returning a u64 in milliseconds? No, the return type is Duration. Actually the code shown in the diff doesn't show the full lookup_interval_function. Assuming it returns Duration, the next_lookup_interval also returns Duration.

The 1000f64 * backoff_ms suggests backoff_ms is in seconds? No, lower_limit is passed as ITERATIVE_LOOKUP_INITIAL_MS which is 100.0 — likely milliseconds. Then 1000f64 * 100.0 = 100_000 microseconds = 100 milliseconds. So backoff_ms is actually in milliseconds, and the multiplication converts to microseconds for Duration::from_micros.

This is confusing naming — backoff_ms contains milliseconds, but 1000f64 * backoff_ms treats it as if it were seconds? No wait: Duration::from_micros(100_000) = 100ms. And 1000f64 * 100.0 = 100_000. So yes, backoff_ms is milliseconds, multiplied by 1000 to get microseconds.

But lower_limit * 2f64.powi(...) — if lower_limit = 100.0 (ms), then after 1 empty lookup, backoff = 200.0 ms. 1000 * 200.0 = 200_000 microseconds = 200 ms. Correct.

However, from_micros takes u64, and round() as u64 could truncate for very large values. But upper_limit bounds this. Fine.

---

Code Organization

13. netrestrict.rs module is pub but could be pub(crate) (p2p.rs:70)

pub mod netrestrict;

The NetRestrict type is re-exported at p2p.rs:84:

pub use netrestrict::NetRestrict;

Having both pub mod netrestrict and pub use netrestrict::NetRestrict exposes the module and the type. This is redundant. Since IpNet is also re-exported via pub use ipnet::IpNet in netrestrict.rs, clients can use ethrex_p2p::IpNet or ethrex_p2p::netrestrict::IpNet.

Suggestion: Make pub(crate) mod netrestrict and keep the pub use netrestrict::NetRestrict.

14. Test helper table_restricted_to uses FixedAnswer(true) (peer_table.rs:2208-2215)

fn table_restricted_to(cidr: &str) -> PeerTableServer {
    PeerTableServer::new(
        H256::zero(),
        10,
        Box::new(FixedAnswer(true)),  // always passes filter
        NetRestrict::new(vec![cidr.parse().unwrap()]),
    )
}

This is fine for testing netrestrict in isolation, but the name suggests it's testing restricted tables. The FixedAnswer(true) means the fork ID filter always passes. Add a comment that filter behavior is not under test here.

---

Documentation

15. CLI help mentions geth's --netrestrict but behavior differs slightly

The doc comment says "Mirrors geth's --netrestrict". Geth's implementation also applies to net.Listen (binding to specific interfaces), not just filtering. This implementation only filters packets and discovery results, doesn't restrict which local interfaces to bind to.

Suggestion: Clarify that this filters traffic but doesn't affect interface binding.

---

Minor Issues

16. Inconsistent tracing level for netrestrict drops

• UDP packets: trace! (server.rs:388)
• Inbound TCP: debug! (network.rs:226)
• Discovery nodes: trace! (peer_table.rs:1598, 1660)
• Bootnodes: warn! (server.rs:155)

For the same security policy, logging levels should be consistent. debug! or warn! for all would be appropriate.

17. netrestrict field in P2PContext is never used for outbound RLPx dials

Looking at rlpx/initiator.rs, the do_look_for_peer gets a contact from the peer table, which has already been filtered. But what about PeerConnection::spawn_as_initiator — does it check netrestrict? The peer table's get_contact_to_initiate already filters via do_get_contact_to_initiate which checks dial_allowed and contact state. The netrestrict is applied at storage time in insert_if_new and new_contacts. So outbound dials are implicitly filtered.

But what about get_closest_from_pool? Used for discovery FINDNODE responses — returns nodes from pool, which were filtered on insertion. ✓

---

Summary

Priority	Issue	Location	Fix
High	discovered_count overcounts on duplicate inserts	peer_table.rs:604-605	Check insert return value
Medium	Discv4State missing explicit Default for new fields	discv4/server.rs	Add Default impl or verify derive
Medium	Repeated Vec<IpNet> clones	initializers.rs	Create NetRestrict once, clone it
Low	Inconsistent logging levels for drops	Multiple	Standardize on debug! or warn!
Low	pub mod netrestrict redundant with re-export	`p2p.rs:70

---

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

[github-actions]

🤖 Codex Code Review

1. Build break: PeerTableServer::spawn now requires a fourth NetRestrict argument in crates/networking/p2p/peer_table.rs, but the integration tests in test/tests/p2p/discovery/discv5_server_tests.rs and test/tests/p2p/discovery/discv5_server_tests.rs still call the old 3-argument form. As written, this should fail to compile in CI for that test target. Passing NetRestrict::default() there would fix it.

Beyond that, the production-side changes look sound. netrestrict is enforced consistently at bootnode intake, UDP ingress, TCP ingress, and peer-table insertion, and the new dial/discovery backoff logic looks reasonable and non-consensus-affecting.

I couldn’t run the Rust tests here because the environment’s rustup home is read-only and cargo fails before invoking the toolchain.

---

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

[github-actions]

Lines of code report

Total lines added: 461
Total lines removed: 0
Total lines changed: 461

Detailed view

+-----------------------------------------------------------+-------+------+
| File                                                      | Lines | Diff |
+-----------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/cli.rs                                  | 1453  | +14  |
+-----------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs                         | 1081  | +11  |
+-----------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs                      | 545   | +3   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discovery/discv4_handlers.rs | 529   | +20  |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discovery/discv5_handlers.rs | 832   | +26  |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discovery/mod.rs             | 64    | +37  |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discovery/server.rs          | 622   | +52  |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv4/server.rs             | 36    | +2   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discv5/server.rs             | 218   | +4   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/netrestrict.rs               | 68    | +68  |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/network.rs                   | 779   | +8   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/p2p.rs                       | 23    | +2   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/peer_table.rs                | 1782  | +205 |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/initiator.rs            | 122   | +3   |
+-----------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/test_utils.rs                | 418   | +6   |
+-----------------------------------------------------------+-------+------+

[github-actions]

🤖 Claude Code Review

This confirms full coverage. I have enough context to finalize the review.

Review: back off failed dials, pace discovery by saturation and add --p2p.netrestrict

Overall this is a well-motivated, carefully implemented change. The per-node dial backoff replaces a genuinely broken mechanism (FIFO-reset already_tried_peers that retried dead nodes every pool sweep), the saturation-based lookup pacing is correct and well-tested, and --p2p.netrestrict is applied consistently across every insertion path I traced (insert_if_new, do_new_contacts, do_new_contact_records, bootnode filtering, inbound TCP accept, and raw UDP packet routing). Good test coverage of the new arithmetic (dial_backoff, next_lookup_interval, netrestrict CIDR matching).

Findings, roughly in order of importance:

1. discovered_count is shared across both discovery protocols, weakening the saturation signal when both run simultaneously (the default). In discv4_handlers.rs:150-164 / discv5_handlers.rs (mirrored), a lookup is judged "productive" by comparing the peer table's global discovered_count before/after. Since discv4 and discv5 run concurrently by default (cli.rs:578-579 etc.), a discv5-driven insert can make a concurrently-finishing discv4 lookup look productive (and vice versa), resetting empty_lookups_in_a_row even though that specific lookup found nothing new. In the fully-saturated steady state this washes out (neither protocol finds anything, so both correctly back off), but during the transition it can keep one protocol's backoff counter perpetually reset by the other's traffic, delaying the pacing this PR is meant to achieve. Not a correctness bug, but worth a comment noting the limitation, or scoping the counter per-protocol if precision matters here.

2. Doc comment on DIAL_BACKOFF_MAX appears off-by-one (peer_table.rs:798-801): "a node that never answers is retried a sixth time roughly 19 minutes after the fifth" doesn't match the code. dial_backoff(5) (the wait between attempt 5 and attempt 6) is 35s·2^4 = 560s ≈ 9.3 min, not ~19 min. The ~19 min figure is dial_backoff(6) = 35s·2^5 ≈ 18.7 min, i.e. the wait between attempts 6 and 7. Harmless (comment-only), but worth fixing so operators reading it get the right expectation of retry cadence.

3. NetRestrict/IpNet matching is family-strict — an IPv4 CIDR won't match a v4-mapped IPv6 address (::ffff:a.b.c.d) since ipnet::IpNet::contains requires matching address family. If a devnet peer connects over a dual-stack socket that reports such an address, it would be incorrectly rejected even though it's logically inside an allowed IPv4 range. Likely a rare edge case and matches geth's own behavior, but worth a docs note or an explicit test if dual-stack devnets are a supported scenario.

Minor/non-blocking observations:

• cmd/ethrex/initializers.rs and l2/initializers.rs construct NetRestrict::new(opts.netrestrict.clone()) three/two separate times rather than building one NetRestrict and cloning the (cheap, Arc-backed) value — trivial extra allocations at startup only, not worth changing unless you want the tidiness.
• RLPxInitiator::handle_look_for_peer polling at a flat 600ms when there's nothing to dial (rather than waiting for the shortest remaining per-node backoff) is a reasonable simplification, not a bug — just leaves a little slack versus optimal wakeup timing.

No issues found with the RLP/consensus-sensitive paths (discv4/discv5 ENR-seq handling looks correct, and the new was_validated() gate in discv4_handle_ping is a sensible tightening not present before), gas/EVM accounting is untouched by this PR, and I didn't find memory-safety, panic, or overflow issues (dial_backoff's checked_shl/checked_mul chain correctly saturates to the cap for large failure counts).

---

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