Skip to content

Conversation

@ElFantasma
Copy link
Contributor

Motivation

Per the discv5 spec, when receiving a PONG with an ENR sequence number different from our
cached version, we should request the updated ENR using FINDNODE with distance 0. This
ensures we have up-to-date ENRs for peers (IP changes, capability updates, etc.).

Description

  • Implements ENR update request in handle_pong when PONG's enr_seq differs from cached
    value
  • Sends FINDNODE with distance 0 to request the peer's current ENR
  • Adds unit test test_enr_update_request_on_pong verifying:
    • PONG with matching enr_seq does NOT trigger FINDNODE
    • PONG with different enr_seq DOES trigger FINDNODE

Closes #5850

ElFantasma and others added 30 commits December 9, 2025 10:31
**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves #111, Resolves #222 -->

**Checklist**

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

Closes #5574 and #5575.

Co-authored-by: Esteban Dimitroff Hodi <[email protected]>
**Checklist**

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

Closes #5580 and #5581
**Checklist**

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

Closes #5578 and #5579
**Checklist**

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

Closes #5576 and closes #5577
**Checklist**

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

Closes #issue_number
**Motivation**
I saw current new_nonce impl allocated a vector when it can just return
a fixed size array.

**Description**

Removes the needless vec

**Checklist**

- [ ] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR
includes breaking changes to the `Store` requiring a re-sync.
**Motivation**

In order to start merging discv5 code into main, to avoid having a huge
PR at the end of the development, we should create a feature flag
disabled by default.

Closes #5639
**Checklist**

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

Closes #5586
Closes #5587
**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves #111, Resolves #222 -->

**Checklist**

- [ ] Updated `STORE_SCHEMA_VERSION` (crates/storage/lib.rs) if the PR
includes breaking changes to the `Store` requiring a re-sync.
**Motivation**

<!-- Why does this pull request exist? What are its goals? -->

**Description**

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves #111, Resolves #222 -->

**Checklist**

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

Closes #5570
Closes #5571
**Checklist**

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

Closes #5566
Closes #5567
@github-actions
Copy link

github-actions bot commented Feb 2, 2026

Kimi AI Code Review

This PR aims to improve the handling of ENR updates in the DiscoveryServer by requesting updated ENRs when a PONG message has a different enr_seq than the cached version. Here's a detailed review:

Code Correctness and Potential Bugs

  1. Line 473-493: The code correctly checks if the enr_seq from the PONG message differs from the cached sequence number. If they differ, it requests an updated ENR by sending a FINDNODE message with a distance of 0. This logic seems correct.

  2. Line 882-898: The test case test_enr_update_request_on_pong is added to verify the new functionality. The test creates a local and remote node, adds the remote node to the peer table, and then tests the behavior of the handle_pong method with PONG messages having different enr_seq values. The test appears to be comprehensive and correctly checks the expected behavior.

Security Vulnerabilities

  1. Line 473-493: The code does not introduce any obvious security vulnerabilities. It correctly handles the case where the enr_seq differs, which is important for maintaining an up-to-date view of the network.

Performance Implications

  1. Line 473-493: Requesting updated ENRs when the sequence number differs could lead to additional network traffic. However, this is necessary to ensure the local node has the most recent information about remote nodes. The performance impact should be minimal as long as the frequency of ENR updates is reasonable.

Rust Best Practices and Idiomatic Patterns

  1. Line 473-493: The code follows Rust best practices by using await? for asynchronous operations and handling potential errors. The use of trace! for logging is also appropriate.

Memory Safety and Proper Error Handling

  1. Line 473-493: The code correctly handles potential errors by using await?. It also checks for None values before accessing fields, which prevents panics due to unwrapping None.

Code Readability and Maintainability

  1. Line 473-493: The code is well-structured and easy to understand. The comments explaining the logic are helpful.

  2. Line 882-898: The test case is well-named and clearly describes its purpose. The test setup is detailed and ensures the test covers the intended scenario.

Ethereum-specific Considerations

  1. Line 473-493: The code does not directly interact with EVM opcodes, gas accounting, consensus rules, or RLP encoding/decoding. However, maintaining accurate ENR information is crucial for the overall health of the Ethereum network, as it affects node discovery and connectivity.

Overall, this PR looks good. It introduces a necessary improvement to handle ENR updates correctly and includes a comprehensive test case to verify the new functionality. The code follows Rust best practices and is well-structured for readability and maintainability. I approve this PR with no changes requested.


Automated review by Kimi (Moonshot AI)

Base automatically changed from discv5-server-rate-limit to main February 9, 2026 21:26
@ilitteri ilitteri added this pull request to the merge queue Feb 11, 2026
@ilitteri ilitteri removed this pull request from the merge queue due to a manual request Feb 11, 2026
@ElFantasma ElFantasma added this pull request to the merge queue Feb 11, 2026
Merged via the queue into main with commit 1a47fb1 Feb 11, 2026
52 checks passed
@ElFantasma ElFantasma deleted the discv5-server-enr-update-on-pong branch February 11, 2026 15:31
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Feb 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Request updated ENR when PONG enr_seq is higher than cached

6 participants