31/WAKU2-ENR: Waku v2 usage of ENR #465

D4nte · 2021-10-15T03:54:15Z

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

oskarth

Thanks! Generally looks reasonable as a raw spec. Some comments on content, as well as how this fits with existing spec and usage.

oskarth · 2021-10-20T07:01:30Z

content/docs/rfcs/31/README.md

+
+# Abstract
+
+This RFC describes the usage of the ENR (Ethereum Node Records) format for [10/WAKU2](/specs/10) purposes.


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?

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.

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 of multiaddr for discovery? That way it won't affect spec lifecycle management.

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)

oskarth · 2021-10-20T07:07:19Z

content/docs/rfcs/31/README.md

+- 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.


Probably worth mentioning where this key comes from

oskarth · 2021-10-20T07:08:39Z

content/docs/rfcs/31/README.md

+
+## Limitations
+
+Supported key type is `secp256k1` only.


Maybe mention briefly why and specifically that it doesn't support common ed25519 (assuming this is the standard often used)

Added a comment.

Do note that, afaik, this limitation could easily be overcame with further RFCs.

kdeme · 2021-10-20T07:29:37Z

content/docs/rfcs/31/README.md

+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


This sections is more like the "rationale" of why multiaddr. Perhaps adjust title?

kdeme · 2021-10-20T07:33:53Z

content/docs/rfcs/31/README.md

+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


:100: becomes 💯 in github markdown :)

kdeme · 2021-10-20T07:39:34Z

content/docs/rfcs/31/README.md

+- 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.


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?

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.

content/docs/rfcs/31/README.md

jm-clius · 2021-10-25T08:15:23Z

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

Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>

wemeetagain · 2021-10-28T18:19:24Z

content/docs/rfcs/31/README.md

+  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


is it worth abbreviating this key and saving a few bytes?

oskarth

LGTM as raw; still think we need to clean up general discovery specs here

jm-clius

Agree that this is good as a guideline for ENR usage, even if how this is merged with discovery spec is TBD.

oskarth · 2021-11-11T04:00:32Z

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

wip: Define ENR extension

68f5058

Add definition and usage

015b884

D4nte changed the title ~~[wip] 31/WAKU2-ENR: Waku v2 usage of ENR~~ 31/WAKU2-ENR: Waku v2 usage of ENR Oct 20, 2021

D4nte added 2 commits October 20, 2021 13:07

Fix references anchor

ff65af9

Typos

08c076c

D4nte marked this pull request as ready for review October 20, 2021 02:11

D4nte requested review from jm-clius and oskarth October 20, 2021 02:12

D4nte mentioned this pull request Oct 20, 2021

25/LIBP2P-DNS-DISCOVERY: Leaf entry format to allow for multiple mutliaddrs and self signature #462

Closed

oskarth requested a review from dryajov October 20, 2021 06:59

oskarth reviewed Oct 20, 2021

View reviewed changes

kdeme reviewed Oct 20, 2021

View reviewed changes

D4nte added 2 commits October 25, 2021 13:33

Action feedback

3656da4

Small corrections

880b535

jm-clius reviewed Oct 25, 2021

View reviewed changes

content/docs/rfcs/31/README.md Outdated Show resolved Hide resolved

Clarify that the 3 multiaddresses section

db88d72

D4nte mentioned this pull request Oct 26, 2021

Implement EIP-1459 for basic node discovery waku-org/js-waku#75

Closed

Typo

694c2c9

Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>

D4nte force-pushed the enr-usage branch from 4da5bc4 to 694c2c9 Compare October 26, 2021 06:49

jm-clius mentioned this pull request Oct 26, 2021

Support for multiaddrs in ENR waku-org/nwaku#755

Closed

D4nte mentioned this pull request Oct 27, 2021

Add multiaddr to ENR ChainSafe/discv5#142

Closed

wemeetagain reviewed Oct 28, 2021

View reviewed changes

oskarth approved these changes Nov 1, 2021

View reviewed changes

jm-clius approved these changes Nov 1, 2021

View reviewed changes

jm-clius mentioned this pull request Nov 1, 2021

Release nim-waku v0.6 waku-org/nwaku#725

Closed

11 tasks

This was referenced Nov 8, 2021

Proposal: ENR filtering on Waku-field in discv5 waku-org/nwaku#763

Merged

transport protocol specification #471

Merged

oskarth merged commit 55add95 into master Nov 11, 2021

oskarth deleted the enr-usage branch November 11, 2021 04:00

oskarth mentioned this pull request Nov 11, 2021

Clean up 31/WAKU2-ENR spec #473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

31/WAKU2-ENR: Waku v2 usage of ENR #465

31/WAKU2-ENR: Waku v2 usage of ENR #465

D4nte commented Oct 15, 2021 •

edited

Loading

oskarth left a comment

oskarth Oct 20, 2021

D4nte Oct 25, 2021 •

edited

Loading

jm-clius Oct 25, 2021

D4nte Oct 25, 2021

oskarth Oct 20, 2021

oskarth Oct 20, 2021

D4nte Oct 25, 2021

kdeme Oct 20, 2021

kdeme Oct 20, 2021

kdeme Oct 20, 2021

D4nte Oct 25, 2021

jm-clius commented Oct 25, 2021 •

edited

Loading

wemeetagain Oct 28, 2021

oskarth left a comment

jm-clius left a comment

oskarth commented Nov 11, 2021


		# Abstract

		This RFC describes the usage of the ENR (Ethereum Node Records) format for [10/WAKU2](/specs/10) purposes.

31/WAKU2-ENR: Waku v2 usage of ENR #465

31/WAKU2-ENR: Waku v2 usage of ENR #465

Conversation

D4nte commented Oct 15, 2021 • edited Loading

oskarth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D4nte Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jm-clius commented Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

oskarth left a comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

oskarth commented Nov 11, 2021

D4nte commented Oct 15, 2021 •

edited

Loading

D4nte Oct 25, 2021 •

edited

Loading

jm-clius commented Oct 25, 2021 •

edited

Loading