-
Notifications
You must be signed in to change notification settings - Fork 15
31/WAKU2-ENR: Waku v2 usage of ENR #465
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Generally looks reasonable as a raw spec. Some comments on content, as well as how this fits with existing spec and usage.
|
||
# Abstract | ||
|
||
This RFC describes the usage of the ENR (Ethereum Node Records) format for [10/WAKU2](/specs/10) purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a sentence on two on what it is used for. If it is just for Discovery, maybe we can be explicit about that.
I wonder if it makes sense to have a spec only to say "use ENR" and then this https://rfc.vac.dev/spec/25/ spec which is now outdated. Perhaps these should live in the same spec, which explains (1) Use of EIP-1459 (2) Extension with EIP-778 and specifics mentioned in this spec. Then optionally as future simplification work (3) multiaddr all the way (with a brief sentence on why etc).
Right now it is quite hard to follow with existing eip1459 usage, the libp2p peer discovery via dns spec, and now this.
@jm-clius wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keen to merge this in RFC-25 and move RFC-25 towards what you propose:
- Use of EIP-1459
- Extension with EIP-778 and specifics mentioned in this spec
- Mention multiaddr all the way (with a brief sentence on why etc) as possible extension.
Waiting for @jm-clius 's input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaics we'll only use this for discovery purposes. Also keen that this be merged with RFC-25 somehow. Just bear in mind that RFC 25 proposes a DNS-discovery method specifically whereas this is (and should be) more general. We'll likely use ENR for discv5 as well, perhaps for rendezvous, etc.
This raises two questions:
- What should the new, consolidated spec be called?
I suggest something like Waku v2 discoverable addresses - How do we manage spec lifecycle of the included RFC 25 method is never implemented?
Perhaps the whole of the current RFC 25 can be relegated to an example use case ofmultiaddr
for discovery? That way it won't affect spec lifecycle management.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having discussed with @jm-clius yesterday (hence the comment above), I think it may make more sense to keep this RFC separate because:
- We intend to use ENR in at least two different (discovery) context: discv5 and EIP-1459
- If we move this to RFC-25 then it is unclear how we can manage the maturity of the spec if we don't end-up implementing the multiaddr discovery currently described in RFC-25
If the issue is that RFC-25 is now deprecated because we are not using a multiaddr tree but an ENR tree then I'd suggest to have a similar structure to the EIPs:
- RFC-31 for ENR Usage (EIP-788 for Waku)
- RFC-25 for ENR tree usage (EIP-1459 for Waku)
content/docs/rfcs/31/README.md
Outdated
- The value MUST be a list of binary encoded multiaddr prefixed by their size. | ||
- The size of the multiaddr MUST be encoded in a Big Endian unsigned 16-bit integer. | ||
- The size of the multiaddr MUST be encoded in 2 bytes. | ||
- The `secp256k1` MUST be present on the record. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth mentioning where this key comes from
|
||
## Limitations | ||
|
||
Supported key type is `secp256k1` only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe mention briefly why and specifically that it doesn't support common ed25519 (assuming this is the standard often used)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment.
Do note that, afaik, this limitation could easily be overcame with further RFCs.
content/docs/rfcs/31/README.md
Outdated
Currently, Waku v2 nodes running in a Browser only support websocket transport protocol. | ||
Hence, new ENR keys needs to be defined to allow for the encoding of transport protocol other than raw TCP. | ||
|
||
## Usage of multiaddr format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sections is more like the "rationale" of why multiaddr. Perhaps adjust title?
content/docs/rfcs/31/README.md
Outdated
Alice is a node operator, she runs a node that supports inbound connection for the following protocols: | ||
- TCP 10101 on 1.2.3.4 | ||
- UDP 20202 on 1.2.3.4 | ||
- TCP 30303 on 1234:5600:100:1::142 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:100:
becomes 💯 in github markdown :)
content/docs/rfcs/31/README.md
Outdated
- The node's peer id SHOULD be deduced from the `secp256k1` value. | ||
- The multiaddresses SHOULD NOT contain a peer id. | ||
- TCP, UDP, IP (IPv4 and IPv6) connection details SHOULD be encoded using the respecting pre-defined ENR keys `tcp`, `udp`, `ip` (and `tcp6`, `udp6`, `ip6` for IPv6); | ||
Multiaddr format can be large, using the predefined keys when available allows more connection details to be encoded in one ENR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if ip
is set to 1.2.3.4
and that is also the address to reach the websockets endpoint, is it allowed to encode a multiaddr like /ipv4//tcp/443/wss
(or even /ipv4//tcp//wss
if tcp
is set to 443
)? As that is an invalid multiaddr I assume?
Or was this bullet point meant only for leaving out the peer id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed the RFC is unclear. Will clarify. Thanks.
What I meant to say is that for pure TCP/UPD connection details, then just use tcp/udp/ip ENR keys.
For websocket, the full multiaddr is needed as the ip/fqdn needs to related to the TLS cert that secure the websocket connection.
In general LGTM. Also agree with consolidating this with RFC 25, as long as we can find a clean way to integrate the DNS-discovery specifics of RFC 25 into this more general addressing spec. See my comment here |
No new RFC would be needed to support encoding other transport protocols in an ENR. | ||
- multiaddr contains both the host and port information, allowing the ambiguity previously described to be resolved. | ||
|
||
# `multiaddrs` ENR key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth abbreviating this key and saving a few bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as raw; still think we need to clean up general discovery specs here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that this is good as a guideline for ENR usage, even if how this is merged with discovery spec is TBD.
I'm gonna go ahead and merge this as we have other work pending on it. All the comments above are still valid and should be addressed in some form in a follow up clean up PR. cc @D4nte as he's away but will be back in December or so |
Proposed usage for ENR for Waku v2 nodes. In particular on how to solve encoding of websockets data in ENR.
js-waku implementation: waku-org/js-waku#324
Resolves #462