-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat(service): add wildcard address resolver #1099
Conversation
Is it possible to add a |
Not a big fan tbh. Maybe we should always add it to the switch. |
@@ -0,0 +1,216 @@ | |||
# Nim-LibP2P | |||
# Copyright (c) 2024 Status Research & Development GmbH |
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.
FYI: We should change this to IFT. I'll sync with legal and come back.
We can change this in a follow up PR.
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.
Should we change all nim-libp2p files?
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.
Ill get back once legal gets back to me.
I asked that question :).
In any case, we can do that in a follow up PR.
Is there a reason to model the resolver as a service? |
The motivation was to make it optional. It might be a breaking change if we make it the default behavior tho. |
With our plan to keep only the |
But I think keeping it as a |
8edbd74
to
03f72a8
Compare
The last commits that enabled the service by default were reverted as they caused strange errors when running the tests. Initially, it was nil access errors in the tor transport test. After those were fixed, random tests failed as the transports were not closed properly. This happened when running on my macOS m1. On CI the test would hang as it's possible to see in the commits. |
I added back the wildcard service enabled by default. There were some issues in the tests that I had to fix, but it seems that the weird errors happened cause of what was explained here #1105. |
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. Thank you.
Let's merge this and open an issue for a deeper investigation of the error described in #1105
tests/transport-interop/Dockerfile
Outdated
@@ -11,6 +11,6 @@ COPY . nim-libp2p/ | |||
|
|||
RUN \ | |||
cd nim-libp2p && \ | |||
nim c --skipProjCfg --skipParentCfg --NimblePath:./nimbledeps/pkgs -p:nim-libp2p -d:chronicles_log_level=WARN -d:chronicles_default_output_device=stderr --threads:off ./tests/transport-interop/main.nim |
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.
Why is this part of this PR?
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.
Don't know what's going on. Seems something went wrong during the reverts and cherry-picks. Sorry, I'm gonna fix it.
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.
no problem; thanks for clarifying.
7837885
to
ae9cb4d
Compare
ae9cb4d
to
1bb1c1a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1099 +/- ##
==========================================
- Coverage 84.53% 84.48% -0.06%
==========================================
Files 91 92 +1
Lines 15517 15640 +123
==========================================
+ Hits 13118 13213 +95
- Misses 2399 2427 +28
|
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.
Let's address @lchenut comments and merge :).
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.
Looks good to me!
Summary
This PR adds a service that expands wildcard addresses to their respective interface addresses equivalent.
closes #1087
Context
Users of nim-libp2p often listen to wildcard addresses like 0.0.0.0 (IPv4) and [::] (IPv6) to allow the OS to bind the transports to all available network interfaces. This is useful for applications that need to be accessible on multiple networks without specifying each interface.
When listening on wildcard addresses, the OS binds to all available IP addresses, simplifying network configuration but requiring a mechanism to resolve these addresses to their specific network interfaces for communication with other peers.
The
listenAddrs
field contains addresses the node listens on, which may include wildcard and private addresses (not directly reachable). Theaddrs
field contains resolved addresses that other peers can use to connect, including public-facing NAT and port-forwarded addresses.Before this PR, nim-libp2p didn't offer a way to resolve wildcard addresses in the
addrs
field.This was reported on status-im/nimbus-eth2#6060 and #1087.
Changes