Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support ipv6 #167

Closed
samlior opened this issue Mar 22, 2022 · 3 comments · Fixed by #245
Closed

Support ipv6 #167

samlior opened this issue Mar 22, 2022 · 3 comments · Fixed by #245
Assignees

Comments

@samlior
Copy link
Contributor

samlior commented Mar 22, 2022

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)

@samlior
Copy link
Contributor Author

samlior commented Mar 22, 2022

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?

@dapplion
Copy link
Contributor

dapplion commented Mar 23, 2022

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

@samlior
Copy link
Contributor Author

samlior commented Mar 23, 2022

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants