[MegaRedHand]

Motivation

DiscoveryConfig::initial_lookup_interval is dead data. #6511 (kademlia
k-bucket routing table v2) replaced the configurable bound in
DiscoveryServer::get_lookup_interval with its own constants, because
each iterative lookup emits ~16 FindNode messages instead of 1 and
needed a rescaled schedule:

         lookup_interval_function(
             peer_completion,
-            self.config.initial_lookup_interval,
-            super::LOOKUP_INTERVAL_MS,
+            ITERATIVE_LOOKUP_INITIAL_MS,   // 500.0
+            ITERATIVE_LOOKUP_INTERVAL_MS,  // 10_000.0
         )

The field itself survived, still threaded from --p2p.lookup-interval
through P2PContext and start_network into a DiscoveryServer that
never reads it. The only live consumer of the flag is the RLPx
initiator, so the help text was also misleading.

While tracing it, a second problem turned up: impl Default for Options
sets lookup_interval: Default::default(), which is 0.0 for f64
rather than the INITIAL_LOOKUP_INTERVAL_MS that clap uses for the
flag. default_l1(), default_l2() and the L2 node options all build
on that impl, so they hand the RLPx initiator a 0ms lower bound. With no
peers connected the easing curve returns Duration::ZERO, and
handle_look_for_peer re-arms its timer with no delay at all. Present
since #5518 added the flag.

Description

• Remove initial_lookup_interval from DiscoveryConfig and stop
  overriding it in start_network. DiscoveryConfig's Default impl
  goes with it: both remaining fields are set explicitly at the single
  construction site, and clap already defaults them to true.
• Reword --p2p.lookup-interval's help to describe what it actually
  controls, and refresh the now-stale doc comments on
  INITIAL_LOOKUP_INTERVAL_MS / LOOKUP_INTERVAL_MS /
  lookup_interval_function.
• Default Options::lookup_interval to INITIAL_LOOKUP_INTERVAL_MS.

No behavior change for the discovery server, which was already running
on its own constants. The second commit does change behavior for the
dev/L2 paths that go through Options::default(), restoring the 100ms
floor they were meant to have.

Checklist

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[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]

🤖 Claude Code Review

Confirmed, no leftover DiscoveryConfig::default() calls or stale references anywhere. Everything is consistent and the removal is exhaustive.

Review: PR 7163 — drop unused discovery lookup interval config

This is a clean, well-scoped dead-code removal plus a real bug fix. I verified the changes against the full call graph (P2PContext::initial_lookup_interval → RLPx initiator, DiscoveryConfig construction sites, Options::default()), and everything checks out.

Correctness

• The core fix (cli.rs:595, lookup_interval: INITIAL_LOOKUP_INTERVAL_MS instead of Default::default()) is a genuine bug fix. Confirmed: f64::default() is 0.0, and this value flows through P2PContext::initial_lookup_interval → RlpxInitiator::get_lookup_interval → lookup_interval_function as the lower bound. With zero peers, progress = 0, so the easing curve returns the lower bound itself — Duration::ZERO — causing the initiator to busy-loop reconnection attempts with no backoff. Good catch, and the fix is correct and minimal.
• The DiscoveryConfig field removal is exhaustive — checked all construction sites (initializers.rs:382, discovery/server.rs:457, network.rs:137-141) and confirmed no remaining references to DiscoveryConfig::default() or initial_lookup_interval on that struct. The field on P2PContext (a distinct, still-live field feeding the RLPx initiator) is correctly left untouched.
• No behavior change for the discovery server itself, consistent with the PR description — it already used its own ITERATIVE_LOOKUP_* constants post-feat(l1): kademlia k-bucket routing table v2 #6511, so this override was dead regardless of its value.

Style / clarity

• Updated doc comments on INITIAL_LOOKUP_INTERVAL_MS/LOOKUP_INTERVAL_MS/lookup_interval_function and the CLI help text accurately reflect actual usage now (RLPx initiator only, not discv4/discv5).
• Splitting into two commits (mechanical removal, then the default-value fix) makes the bug fix easy to review in isolation — good practice.

Minor observations (not blocking)

• No test added asserting Options::default().lookup_interval != 0.0 or covering lookup_interval_function at the boundary (progress = 0 with a non-zero lower bound). Given the bug was silent for a while (since feat(l1): improved lookup interval #5518), a small regression test would be cheap insurance, but it's a reasonable judgment call to skip for a one-line default-value fix.
• The unchecked PR checklist item about STORE_SCHEMA_VERSION is correctly not applicable here since there's no storage schema change.

No correctness, security, or consensus-rule concerns — this only touches peer-discovery/connection pacing, not block/state validation.

---

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

[github-actions]

🤖 Kimi Code Review

Review Summary
Clean refactor separating RLPx connection intervals from discovery lookup intervals. Removes configurability from discovery (using safe constants) while keeping RLPx intervals tunable. No critical security issues, but verify RLPx initiator still receives the CLI value.

Detailed Feedback

1. Potential dead code in context (cmd/ethrex/network.rs)
The context struct likely contains initial_lookup_interval field that was previously passed to DiscoveryConfig (line 140 in original). Since this override is removed, verify:

• The RLPx initiator reads this value directly from context (not shown in diff)
• If unused, remove the field from context to avoid confusion

2. DiscoveryConfig Default implementation removal (crates/networking/p2p/discovery/mod.rs)
Removing Default is good for explicit configuration, but verify no other test files or modules use DiscoveryConfig::default(). The compiler will catch this, but worth checking integration tests.

3. Comment accuracy (crates/networking/p2p/discovery/mod.rs)
Lines 30-33: The comment states discovery "has its own bounds" but the constants INITIAL_LOOKUP_INTERVAL_MS and LOOKUP_INTERVAL_MS remain in this module. If discovery uses different hardcoded values elsewhere, consider moving these constants to the RLPx module to avoid confusion, or clarify that these constants are now RLPx-specific.

4. CLI help text improvement (cmd/ethrex/cli.rs)
Line 459: Good clarification. The new text accurately describes this affects "RLPx connection attempts" rather than discovery.

5. Explicit struct initialization (cmd/ethrex/initializers.rs)
Line 382-385: Good removal of ..Default::default(). With only two boolean fields, explicit initialization is clearer and safer.

6. No regression in configurability
Ensure the RLPx initiator component (not shown in diff) actually uses the lookup_interval value from CLI options. If it was previously reading from DiscoveryConfig.initial_lookup_interval, it will now need to source this from the context or CLI options directly.

Minor Suggestion
Consider renaming INITIAL_LOOKUP_INTERVAL_MS to INITIAL_RLPX_INTERVAL_MS since it's no longer used by discovery, making the code self-documenting. However, this is optional given the comments are clear.

Security Assessment

• Positive: Removing configurable discovery intervals prevents users from accidentally setting overly aggressive values that could trigger rate limiting or blacklisting by peers.
• Positive: 100ms floor (10 conn/sec) for RLPx is reasonable and won't cause amplification issues.

Verdict
Approve with the caveat that you verify the RLPx initiator still receives the configured interval value through a different mechanism (since it's no longer in DiscoveryConfig).

---

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

[github-actions]

🤖 Codex Code Review

Findings

• Minor: crates/networking/p2p/discovery/server.rs has a stale doc comment. It still says the test helper uses a "dummy initial lookup interval", but that field was removed from DiscoveryConfig in this PR.

No material correctness, security, performance, or Ethereum-protocol issues stood out in the actual behavior change. The patch looks sound: cmd/ethrex/cli.rs now aligns Options::default() with the CLI default, and crates/networking/p2p/network.rs no longer leaks the RLPx-specific knob into discovery-server pacing.

I could not run cargo check in this environment because rustup tried to write under /home/runner/.rustup and failed on the read-only filesystem, so compile verification is still a gap.

---

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

[github-actions]

Lines of code report

Total lines added: 0
Total lines removed: 15
Total lines changed: 15

Detailed view

+--------------------------------------------------+-------+------+
| File                                             | Lines | Diff |
+--------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/initializers.rs                | 979   | -1   |
+--------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discovery/mod.rs    | 24    | -10  |
+--------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/discovery/server.rs | 410   | -1   |
+--------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/network.rs          | 724   | -3   |
+--------------------------------------------------+-------+------+