chore(p2p): changes necessary by swarm-network-rewrite #16898 #2051
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces changes necessary for the swarm-network-rewrite effort (ethereum#16898), focusing on improving the p2p simulation infrastructure. The changes enhance network simulation capabilities by making pipe connections configurable, improving node configuration handling, and standardizing logging patterns.
Changes:
- Introduced a new
pipespackage to centralize TCP and net.Pipe implementations for simulation testing - Enhanced node configuration with Port field and made pipe connections configurable (NetPipe vs TCPPipe)
- Standardized error logging to use structured key-value format throughout the simulation code
- Added metrics tracking for peer message sending in the protocols package
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| p2p/simulations/pipes/pipes.go | New package providing NetPipe and TCPPipe functions for simulation connections |
| p2p/simulations/network_test.go | Updated to use RandomNodeConfig for node creation |
| p2p/simulations/network.go | Moved GetNodes method for better code organization |
| p2p/simulations/mocker.go | Improved error handling and updated logging to structured format |
| p2p/simulations/http_test.go | Updated tests to use RandomNodeConfig before creating nodes |
| p2p/simulations/http.go | Changed CreateNode to accept configurable NodeConfig instead of always generating random config |
| p2p/simulations/adapters/types.go | Added Port field to NodeConfig and assignTCPPort function for port allocation |
| p2p/simulations/adapters/inproc_test.go | New comprehensive test suite for TCPPipe and NetPipe functionality |
| p2p/simulations/adapters/inproc.go | Made pipe connections configurable, added NewTCPAdapter, fixed service registration error handling |
| p2p/simulations/adapters/exec.go | Refactored ExternalIP into reusable function with better error handling |
| p2p/simulations/adapters/docker.go | Extracted ErrLinuxOnly as package variable and added Port configuration |
| p2p/rlpx_test.go | Migrated tcpPipe function to shared pipes package |
| p2p/protocols/protocol.go | Added metrics for tracking peer message sending |
Comments suppressed due to low confidence (1)
p2p/simulations/adapters/inproc.go:63
- The NewTCPAdapter function should have a documentation comment explaining its purpose and how it differs from NewSimAdapter. This helps developers understand when to use TCPPipe vs NetPipe for simulations.
func NewTCPAdapter(services LifecycleConstructors) *SimAdapter {
return &SimAdapter{
pipe: pipes.TCPPipe,
nodes: make(map[discover.NodeID]*SimNode),
lifecycles: services,
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return 0, err | ||
| } | ||
| l.Close() |
There was a problem hiding this comment.
The listener should be closed before attempting to parse the port to avoid a potential race condition. If another process binds to the same port between l.Close() and when the port is actually used, there could be a conflict. Consider keeping the listener open longer or document this limitation.
| l.Close() | |
| defer l.Close() |
|
|
||
| func setSocketBuffer(conn net.Conn, socketReadBuffer int, socketWriteBuffer int) error { | ||
| switch v := conn.(type) { | ||
| case *net.UnixConn: | ||
| err := v.SetReadBuffer(socketReadBuffer) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| err = v.SetWriteBuffer(socketWriteBuffer) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The setSocketBuffer function is defined but never called. If this function is intended for future use, consider adding a comment explaining its purpose. Otherwise, consider removing it to avoid dead code.
| func setSocketBuffer(conn net.Conn, socketReadBuffer int, socketWriteBuffer int) error { | |
| switch v := conn.(type) { | |
| case *net.UnixConn: | |
| err := v.SetReadBuffer(socketReadBuffer) | |
| if err != nil { | |
| return err | |
| } | |
| err = v.SetWriteBuffer(socketWriteBuffer) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| } |
| return nil, err | ||
| } | ||
| // this is simulated 'listening' | ||
| // asynchronously call the dialed destintion node's p2p server |
There was a problem hiding this comment.
Typo in comment: 'destintion' should be 'destination'.
| // asynchronously call the dialed destintion node's p2p server | |
| // asynchronously call the dialed destination node's p2p server |
| panic("Could not startup node network for mocker") | ||
| select { | ||
| case <-quit: | ||
| //error may be due to abortion of mocking; so the quit channel is closed |
There was a problem hiding this comment.
The comment should follow standard Go comment conventions - it should be a complete sentence starting with a capital letter and ending with a period. Consider: "Error may be due to abortion of mocking; so the quit channel is closed."
| //error may be due to abortion of mocking; so the quit channel is closed | |
| // Error may be due to abortion of mocking, so the quit channel is closed. |
| } | ||
| } | ||
| log.Warn("unable to determine explicit IP address, falling back to loopback") | ||
| return net.IP{127, 0, 0, 1} |
There was a problem hiding this comment.
The fallback IP address should use net.IPv4 for better clarity and consistency with Go conventions. Consider: return net.IPv4(127, 0, 0, 1)
| return net.IP{127, 0, 0, 1} | |
| return net.IPv4(127, 0, 0, 1) |
| // ExternalIP gets an external IP address so that Enode URL is usable | ||
| func ExternalIP() net.IP { | ||
| addrs, err := net.InterfaceAddrs() | ||
| if err != nil { | ||
| log.Crit("error getting IP address", "err", err) | ||
| } | ||
| for _, addr := range addrs { | ||
| if ip, ok := addr.(*net.IPNet); ok && !ip.IP.IsLoopback() && !ip.IP.IsLinkLocalUnicast() { | ||
| return ip.IP | ||
| } | ||
| } | ||
| log.Warn("unable to determine explicit IP address, falling back to loopback") | ||
| return net.IP{127, 0, 0, 1} |
There was a problem hiding this comment.
The ExternalIP function should prefer IPv4 addresses or be more specific about which IP version to return. Currently, it may return an IPv6 address when callers expect IPv4, which could cause issues with P2P.ListenAddr formatting. Consider filtering for IPv4 addresses specifically or handling both IP versions appropriately.
| // ExternalIP gets an external IP address so that Enode URL is usable | |
| func ExternalIP() net.IP { | |
| addrs, err := net.InterfaceAddrs() | |
| if err != nil { | |
| log.Crit("error getting IP address", "err", err) | |
| } | |
| for _, addr := range addrs { | |
| if ip, ok := addr.(*net.IPNet); ok && !ip.IP.IsLoopback() && !ip.IP.IsLinkLocalUnicast() { | |
| return ip.IP | |
| } | |
| } | |
| log.Warn("unable to determine explicit IP address, falling back to loopback") | |
| return net.IP{127, 0, 0, 1} | |
| // ExternalIP gets an external IP address so that Enode URL is usable. | |
| // It prefers IPv4 addresses and falls back to IPv6 if no suitable IPv4 is found. | |
| func ExternalIP() net.IP { | |
| addrs, err := net.InterfaceAddrs() | |
| if err != nil { | |
| log.Crit("error getting IP address", "err", err) | |
| } | |
| var ipv6Candidate net.IP | |
| for _, addr := range addrs { | |
| ipNet, ok := addr.(*net.IPNet) | |
| if !ok { | |
| continue | |
| } | |
| ip := ipNet.IP | |
| if ip.IsLoopback() || ip.IsLinkLocalUnicast() { | |
| continue | |
| } | |
| // Prefer IPv4 addresses. | |
| if ip4 := ip.To4(); ip4 != nil { | |
| return ip4 | |
| } | |
| // Remember the first suitable IPv6 address as a fallback. | |
| if ipv6Candidate == nil { | |
| ipv6Candidate = ip | |
| } | |
| } | |
| if ipv6Candidate != nil { | |
| return ipv6Candidate | |
| } | |
| log.Warn("unable to determine explicit IP address, falling back to loopback") | |
| return net.IPv4(127, 0, 0, 1) |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
1 participant
Proposed changes
Ref: ethereum#16898
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which part of the codebase this PR will touch base on,
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that