Conversation
richard-ramos
requested review from
Ivansete-status,
Copilot,
kdeme and
vladopajic
There was a problem hiding this comment.
Pull request overview
This PR adds optional QUIC port support to Ethereum Node Records (ENRs) by extending the existing port infrastructure (TCP, UDP) to include QUIC ports. The implementation follows the same pattern as TCP/UDP ports, including both IPv4 (quic) and IPv6 (quic6) variants in the TypedRecord structure, though the current insertAddress logic only sets the IPv4 variant regardless of IP family (consistent with existing TCP/UDP behavior).
Key Changes:
- Added
quicandquic6fields to the TypedRecord structure for storing QUIC port information - Extended all ENR initialization and update functions to accept an optional
quicPortparameter with backward-compatible overloads - Updated Discovery v5 protocol to support QUIC port in node record creation and logging
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| eth/enr/enr.nim | Added quic/quic6 fields to TypedRecord, extended insertAddress/init/update functions with quicPort parameter and backward-compatible overloads, added "quic"/"quic6" to RLP parser |
| eth/p2p/discoveryv5/protocol.nim | Extended all newProtocol function signatures with enrQuicPort parameter, added multiple backward-compatible overloads, updated logging to include quicPort |
| eth/p2p/discoveryv5/node.nim | Extended node update function with quicPort parameter and backward-compatible overload |
| tests/test_enr.nim | Updated all Record.init and update calls with quicPort parameter, added assertions to verify quic field behavior for IPv4/IPv6 scenarios |
| tests/p2p/test_discoveryv5.nim | Updated newProtocol calls to include quicPort parameter |
| tests/p2p/discv5_test_helper.nim | Updated helper functions to pass quicPort parameter in Record.init and newProtocol calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
richard-ramos
force-pushed
the
feat/quic
branch
3 times, most recently
from
d2506d0 to
c26fdf9
Compare
richard-ramos
force-pushed
the
feat/quic
branch
from
c26fdf9 to
0edf3f4
Compare
kdeme
left a comment
kdeme
left a comment
There was a problem hiding this comment.
I am in the process of adding IPv6 support (#837), which adds parameters similarly like this quic one.
So ideally I get that added together, not to have too many versions of these inits (and having to deprecate them then after).
But at the same time, it also become a bit much as function signature so I will look into different ways of initializing this.
What is that status of the quic usage / addition of CL clients?
Asking this because the quic field in ENR is also not a field with pre-defined meaning in the spec. But it makes sense to add of course.
But for now extraFields could also be used to add this.
| ip: Opt[IpAddress] = Opt.none(IpAddress), | ||
| tcpPort: Opt[Port] = Opt.none(Port), | ||
| udpPort: Opt[Port] = Opt.none(Port), | ||
| quicPort: Opt[Port] = Opt.none(Port), |
There was a problem hiding this comment.
This way of adding a parameter does have the potential of breaking current usages. E.g. when extraFields is passed as parameter but not specifically set with extraFields = ....
But perhaps fine as we control most/all of its usage? Not sure.
| ip: Opt[IpAddress] = Opt.none(IpAddress), | ||
| tcpPort: Opt[Port] = Opt.none(Port), | ||
| udpPort: Opt[Port] = Opt.none(Port), | ||
| quicPort: Opt[Port] = Opt.none(Port), |
we're among the last to implement it |
ok, I meant more regarding its deployment. I know it is rather trivial, but is the |
|
ethereum/consensus-specs#3644 It's out there and used by the majority of clients, so it's maybe a crawler issue? |
Could be, I added it rather quickly and test ran it. I'll verify later. edit: Can confirm it was a mistake on my end. There are indeed a lot of nodes with the |
|
any blockers remaining here? cc @kdeme |
| banNodes, config, rng | ||
| ) | ||
|
|
||
| proc newProtocol*( |
There was a problem hiding this comment.
What is the diff between the two newProtocol procs? Maybe we could have a comment there, or maybe one can be private. Or maybe we could have different proc names for the brand new procs, e.g., newProtocolABC so that we have simpler lookups.
There was a problem hiding this comment.
The difference is the bindIp parameter.
One accepts an IpAddress, the other (newer) one an Opt[IpAddress].
Functionally mostly the same, but with the important (and rather hidden) difference that passing a None there has the effect that this also gets passed down to the newDatagramTransport which underneath looks up for the appropriate "AnyAddress" (:: when dual stack, 0.0.0.0 when IPv4 only).
I want to deprecate the original one, and keep just the newer one with proper comment explaining this.
Nothing really functional no. And also, as I mentioned in the review, it does break the ENR API a bit, but its not too bad as this is mostly (only?) directly used inside discv5 I think. |
| privKey: PrivateKey, | ||
| enrIp: Opt[IpAddress], | ||
| enrTcpPort, enrUdpPort: Opt[Port], | ||
| enrTcpPort, enrUdpPort, enrQuicPort: Opt[Port], |
There was a problem hiding this comment.
We could also do without altering this one (afaik it is not needed for nimbus-eth2 and I'd like to deprecate it).
| proc newProtocol*( | ||
| privKey: PrivateKey, | ||
| enrIp: Opt[IpAddress], | ||
| enrTcpPort, enrUdpPort, enrQuicPort: Opt[Port], |
|
ok, I'll merge this PR and then apply changes similar as in #843 And look into deprecating some of it. |
Adds an optional quic port to ENRs