Skip to content

Conversation

@GrapeBaBa
Copy link
Member

@GrapeBaBa GrapeBaBa commented Sep 2, 2025

  1. This pull request adds support for peer identity and multiaddress handling to the ENR (Ethereum Node Record) implementation, refactors method signatures for improved safety and consistency, and introduces new functionality for QUIC protocol addresses. The changes also include dependency updates and codebase improvements for modularity and maintainability.
  2. Also add more generic read/write ENR function from reader and to writer. At the same time change the file reading function to stream read approach.

also make read only function use const pointer

Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for ENR (Ethereum Node Record) conversion to peer identity and multiaddress handling, enhances the API with safer method signatures using const pointers, and introduces new functionality for QUIC protocol addresses. The changes also improve code modularity by separating file I/O operations from generic reader/writer functions.

Key changes:

  • Added peer ID and multiaddress conversion methods for all ENR types
  • Introduced QUIC protocol support alongside existing UDP functionality
  • Refactored method signatures to use const pointers for read-only operations
  • Split file operations into generic reader/writer functions for better modularity

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/root.zig Refactored file I/O operations into generic reader/writer functions and updated method calls to use new naming conventions
src/enr.zig Added peer ID/multiaddress conversion methods, QUIC support, and changed method signatures to use const pointers for safety
build.zig.zon Added peer-id dependency for peer identity functionality
build.zig Integrated peer-id and multiformats dependencies into the build system

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

src/root.zig Outdated
defer file.close();
/// Generic function to read multiple ENRs from any reader
pub fn readMultipleENRs(reader: anytype, enr_list: *std.ArrayList(ENR), delimiter: []const u8) !void {
const content = try reader.readAllAlloc(enr_list.allocator, std.math.maxInt(usize));
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using std.math.maxInt(usize) as the maximum read size could lead to memory exhaustion attacks. Consider setting a reasonable upper limit for file size to prevent potential DoS vulnerabilities.

Copilot uses AI. Check for mistakes.
src/root.zig Outdated
defer file.close();
/// Generic function to read multiple EncodedENRs from any reader
pub fn readMultipleEncodedENRs(reader: anytype, enr_list: *std.ArrayList(EncodedENR), delimiter: []const u8) !void {
const content = try reader.readAllAlloc(enr_list.allocator, std.math.maxInt(usize));
Copy link

Copilot AI Sep 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using std.math.maxInt(usize) as the maximum read size could lead to memory exhaustion attacks. Consider setting a reasonable upper limit for file size to prevent potential DoS vulnerabilities.

Copilot uses AI. Check for mistakes.
Signed-off-by: Chen Kai <281165273grape@gmail.com>
src/enr.zig Outdated
}

pub fn getQUIC(self: *const Self) !?u16 {
if (self.get("quic")) |udp_bytes| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is reading from a "quic" key but the variable is named udp_bytes, is this desired?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

… char

Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Signed-off-by: Chen Kai <281165273grape@gmail.com>
Copy link
Member

@g11tech g11tech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but @GrapeBaBa we should join ENR, SignableENR and the RLP Enr into just one ENR structure as union over these three tags, so that we dont have to duplicate the functions again and again and do thing zig native way

@g11tech g11tech merged commit 02f1875 into main Sep 3, 2025
1 check passed
@GrapeBaBa
Copy link
Member Author

GrapeBaBa commented Sep 3, 2025

lgtm but @GrapeBaBa we should join ENR, SignableENR and the RLP Enr into just one ENR structure as union over these three tags, so that we dont have to duplicate the functions again and again and do thing zig native way

Which duplicate functions you mean, in the root.zig or enr.zig? I think in general, the caller should use different ENR types in different case, let caller wrap it in an union tagged for use may be extra operation. And I don't see it could reduce codes in different ENR implementation.

If you mean in the root.zig, there are indeed a bit duplicate codes for read/write different ENR. I used anytype, but change it back to multiple functions for more readable. These codes should not be expand more, could refactor it later if need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants