ChainSafe/discv5

Support ipv6

samlior opened this issue · 3 comments

I tried to use this library in an environment where ipv6 and ipv4 co-exist, but encountered some problems:

  1. In UDPTransportService, the socket object is created according to the multiaddr.toOptions().family, which may be udp4 or udp6. When the local socket object is udp4 but the remote host is an ipv6 address, calling this.socket.send will return an error:
Error: send EINVAL aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa
    at doSend (dgram.js:692:16)
    at defaultTriggerAsyncIdScope (internal/async_hooks.js:430:12)
    at afterDns (dgram.js:638:5)
    at processTicksAndRejections (internal/process/task_queues.js:81:21) {
  errno: -22,
  code: 'EINVAL',
  syscall: 'send',
  address: 'aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa',
  port: 12345
}

Maybe we should create and bind two socket objects udp4 and udp6, and then judge which socket object to use when sending?

  1. In many places in the code, enr.getLocationMultiaddr("udp") is used directly without judgment

For example, this place should determine whether message.recipientIp is an ipv4 address or an ipv6 address before calling enr.getLocationMultiaddr("udp"). If message.recipientIp is an ipv6 address, but currentAddr is an ipv4 address, the two cannot be equal, which will cause the local node to keep increasing its own enr.seq

And other places, like here and here and here (Although I don't know what these places are doing)

Actually, I have a small question, I would really appreciate it if someone could answer it for me.

Currently DEFAULT_SEARCH_INTERVAL_MS is 0, and I didn't find the input searchInterval in lodestar's code (maybe I missed it?). If the searchInterval is 0, it means that the node is calling findPeers almost all the time. Will the load be too high when there are more nodes connected?

Currently DEFAULT_SEARCH_INTERVAL_MS is 0, and I didn't find the input searchInterval in lodestar's code (maybe I missed it?). If the searchInterval is 0, it means that the node is calling findPeers almost all the time. Will the load be too high when there are more nodes connected?

In Lodestar we trigger findPeer searches on demand. This option is left there for backwards compatibility since some downstream consumers may rely on discv5 emitting ENRs without any input. However as you note the performance cost is high.

I think we should completely deprecate the interval for findPeers and let the consumer implement a constant loop if that's what they want.

Here we trigger the searches in Lodestar, let me know if that answers your question https://github.com/ChainSafe/lodestar/blob/cdfebc36387aaa3a590217102ca113292f6abb12/packages/lodestar/src/network/peers/discover.ts#L235

EDIT: I got confused for a second too, in Lodestar the Discv5 class directly from src/service, while you are looking at src/libp2p wrapped class that has the searchInterval functionality

Currently DEFAULT_SEARCH_INTERVAL_MS is 0, and I didn't find the input searchInterval in lodestar's code (maybe I missed it?). If the searchInterval is 0, it means that the node is calling findPeers almost all the time. Will the load be too high when there are more nodes connected?

In Lodestar we trigger findPeer searches on demand. This option is left there for backwards compatibility since some downstream consumers may rely on discv5 emitting ENRs without any input. However as you note the performance cost is high.

I think we should completely deprecate the interval for findPeers and let the consumer implement a constant loop if that's what they want.

Here we trigger the searches in Lodestar, let me know if that answers your question https://github.com/ChainSafe/lodestar/blob/cdfebc36387aaa3a590217102ca113292f6abb12/packages/lodestar/src/network/peers/discover.ts#L235

EDIT: I got confused for a second too, in Lodestar the Discv5 class directly from src/service, while you are looking at src/libp2p wrapped class that has the searchInterval functionality

Thanks for the explanation, now I totally get it!