-
Notifications
You must be signed in to change notification settings - Fork 173
Add NAT traversal via UPnP port mapping #771
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: GautamBytes <[email protected]>
@seetadev please review it whenever feasible and suggest improvements! |
examples/upnp/README.md
Outdated
First, ensure you have installed the necessary dependencies from the root of the repository: | ||
|
||
```sh | ||
pip install -e .[upnp] |
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.
This should be:
pip install -e ".[upnp]"
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.
my bad , will update it soon. Any other changes apart from this??
libp2p/discovery/upnp.py
Outdated
except Exception as e: | ||
if str(e) == "Success": | ||
num_devices = 1 |
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.
Is there a specific edge case in miniupnpc
where it raises an exception with the message "Success"
but should actually be treated as a valid discovery result?
This looks a bit fragile, so I was wondering if it's based on something you've seen happen in the wild. Could you clarify the motivation?
logger.exception("UPnP discovery failed") | ||
return False | ||
|
||
async def add_port_mapping(self, port: int, protocol: str = "TCP") -> bool: |
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.
if port <= 0 or port > 65535:
logger.error(f"Invalid port number: {port}")
return False
if port < 1024:
logger.warning(f"Mapping a well-known port: {port} (may fail)")
Setting up these kinds of warning for sketchy port numbers in the params, would save computation time in cases of misbehaviour. Also add similar checks to remove_port_mapping
.
examples/upnp/upnp_demo.py
Outdated
upnp = UpnpManager() | ||
if not await upnp.discover(): | ||
logger.error( | ||
"❌ Could not find a UPnP-enabled gateway. " | ||
"The host will start, but may not be dialable from the public internet." | ||
) | ||
else: | ||
logger.info(f"✅ UPnP gateway found! External IP: {upnp.get_external_ip()}") |
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.
It would be better to integrate the UpnpManager
into the Host
runtime. You can see how the mdns
service is integrated, Unpn
will follow the same thing.
class BasicHost(IHost):
...
def __init__(
self,
network: INetworkService,
enable_mDNS: bool = False,
default_protocols: Optional["OrderedDict[TProtocol, StreamHandlerFn]"] = None,
negotitate_timeout: int = DEFAULT_NEGOTIATE_TIMEOUT,
) -> None:
...
if enable_mDNS:
self.mDNS = MDNSDiscovery(network)
Could go with an enable_upnp
bool to set up the manager object and then inside this part in the example:
async with host.run(listen_addrs=[listen_maddr]):
call host.upnp
to start the relevant operations.
Embedding this in the host logic would be much cleaner and more consistent to updates.
Let me know if you need any pointers on how to set this up.
@seetadev @GautamBytes: Added some inline comments and suggestions. |
Signed-off-by: GautamBytes <[email protected]>
libp2p/__init__.py
Outdated
@@ -277,6 +278,6 @@ def new_host( | |||
|
|||
if disc_opt is not None: | |||
return RoutedHost(swarm, disc_opt, enable_mDNS) | |||
return BasicHost(network=swarm,enable_mDNS=enable_mDNS , negotitate_timeout=negotiate_timeout) | |||
return BasicHost(network=swarm,enable_mDNS=enable_mDNS , negotitate_timeout=negotiate_timeout, enable_upnp=enable_upnp) |
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.
negotitate_timeout
Should be negotiate_timeout
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.
that was already there so didn't notice , will correct it!
logger = logging.getLogger("libp2p.discovery.upnp") | ||
|
||
|
||
class UpnpManager: |
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.
UpnpManager
would benefit with unit tests
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.
Sure , will be adding that asap!
Signed-off-by: GautamBytes <[email protected]>
…tes/py-libp2p into feat/upnp-nat-traversal
What was wrong?
py-libp2p lacked support for automatic port mapping via UPnP, making nodes behind home routers non-dialable from the public internet. This limited peer connectivity in common NAT scenarios.
Issue: NAT traversal for home network users
How was it fixed?
UpnpManager
class that:miniupnpc
viapip install .[upnp]
Summary:
To-Do