fix: use IP addresses instead of dns to store multiaddresses

[richard-ramos]

This is so the circuit relay multiaddrs only use IP and not the long URLs like those used on the fleet.
This has the drawback that if the domain name is updated to point to a different IP, the IP address stored in the multiaddr field will no longer be valid, but I think this is okay, since ENRs are not the ideal place for storing circuit relay addresses (we should use rendezvous instead), and it's done only in an attempt to provide a way for peers to announce themselves.

[status-im-auto]

Jenkins Builds

Click to see older builds (2)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	88f0154	#1	2024-06-19 20:31:49	~1 min	nix-flake	📄log
✔️	0c3a092	#2	2024-06-19 22:16:13	~1 min	nix-flake	📄log

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	652a543	#3	2024-06-19 22:18:42	~1 min	nix-flake	📄log
✔️	4ab846d	#4	2024-06-20 07:23:49	~1 min	nix-flake	📄log

[chaitanyaprem]

As per https://github.com/waku-org/specs/blob/master/standards/core/enr.md#multiaddrs-enr-key , looks like circuit-relay addresses can also be included in ENR.

Are we missing something here? Is there any specific reason we were not including circuit-relay addresses as part of ENR?
cc @jm-clius @fryorcraken

[richard-ramos]

I think go-waku is the only implementation that had support for circuit relay addresses in the ENR. This was later proposed to be added to the specs in this PR: vacp2p/rfc#613

In the code we were including circuit relay addresses in the ENR. The only restriction it had was that the length of the address had to be <= 100 characters which was a somewhat arbitrary number I added while writting that code sometime ago, but, while reviewing that code I found out that condition is not necessary because the code in https://github.com/waku-org/go-waku/blob/master/waku/v2/protocol/enr/localnode.go#L26-L56 will automatically try to write as many multiaddresses as possible in the ENR as long as it does not exceed the 300 bytes limit, which is done when the enr is signed:

• go-waku/waku/v2/protocol/enr/localnode.go

  Line 128 in 795322a

  _ = localnode.Node()

• https://github.com/status-im/go-ethereum/blob/master/p2p/enode/localnode.go#L97
• https://github.com/status-im/go-ethereum/blob/master/p2p/enr/enr.go#L54
• https://github.com/status-im/go-ethereum/blob/master/p2p/enr/enr.go#L285

[chaitanyaprem]

I think go-waku is the only implementation that had support for circuit relay addresses in the ENR. This was later proposed to be added to the specs in this PR: vacp2p/rfc#613

In the code we were including circuit relay addresses in the ENR. The only restriction it had was that the length of the address had to be <= 100 characters which was a somewhat arbitrary number I added while writting that code sometime ago, but, while reviewing that code I found out that condition is not necessary because the code in https://github.com/waku-org/go-waku/blob/master/waku/v2/protocol/enr/localnode.go#L26-L56 will automatically try to write as many multiaddresses as possible in the ENR as long as it does not exceed the 300 bytes limit, which is done when the enr is signed:

* https://github.com/waku-org/go-waku/blob/795322a19665ac48ca1f5cf725f64b1fd1e67808/waku/v2/protocol/enr/localnode.go#L128

* https://github.com/status-im/go-ethereum/blob/master/p2p/enode/localnode.go#L97

* https://github.com/status-im/go-ethereum/blob/master/p2p/enr/enr.go#L54

* https://github.com/status-im/go-ethereum/blob/master/p2p/enr/enr.go#L285

Realized the issue and assumption i was making, thanks for clarifying.

[fryorcraken]

This has the drawback that if the domain name is updated to point to a different IP, the IP address stored in the multiaddr field

I think that's fine. We just need to try it out and we can review if we are in a scenario where IPs change often behind a sepcific domain name. Which is unlikely.

In any case, discv5 and ENR setup needs to work for application on home broadband, behind nat with potential dynamic IPs.

[chaitanyaprem]

LGTM

[chaitanyaprem]

Tested this locally and the ip address is resolved and ENR has ip for circuit-relay address.

[fryorcraken]

This has the drawback that if the domain name is updated to point to a different IP, the IP address stored in the multiaddr field

I think that's fine. We just need to try it out and we can review if we are in a scenario where IPs change often behind a sepcific domain name. Which is unlikely.

In any case, discv5 and ENR setup needs to work for application on home broadband, behind nat with potential dynamic IPs.

One more comment on that. It's actually better to have ip address because the desktop node that includes the address in their ENR needs to include the address of the node they are actually connected to. For example, if the DNS entry resolved to several hosts, then it would be an issue as a 3rd node trying to connected to the desktop node might not hit the right IP.
Same if the IP behind the DNS changes, it does not matter as long as the desktop node advertising itself is still connected to the host with the old ip.

[chaitanyaprem]

This has the drawback that if the domain name is updated to point to a different IP, the IP address stored in the multiaddr field

I think that's fine. We just need to try it out and we can review if we are in a scenario where IPs change often behind a sepcific domain name. Which is unlikely.
In any case, discv5 and ENR setup needs to work for application on home broadband, behind nat with potential dynamic IPs.

One more comment on that. It's actually better to have ip address because the desktop node that includes the address in their ENR needs to include the address of the node they are actually connected to. For example, if the DNS entry resolved to several hosts, then it would be an issue as a 3rd node trying to connected to the desktop node might not hit the right IP. Same if the IP behind the DNS changes, it does not matter as long as the desktop node advertising itself is still connected to the host with the old ip.

That is a valid point, so it makes sense to use IP address only rather than DNS for circuit-relay addresses.
cc @waku-org/nwaku-developers