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

Add UPnP support for Lighthouse #927

Closed
AgeManning opened this issue Mar 17, 2020 · 3 comments
Closed

Add UPnP support for Lighthouse #927

AgeManning opened this issue Mar 17, 2020 · 3 comments
Assignees
Labels
Milestone

Comments

@AgeManning
Copy link
Member

Description

Adding UPnP support will help grow our DHT by allowing NAT traversal for peers with UPnP supported routers.

The modification is relatively trivial, however searching for the gateway and modifying ports should be done asynchronously.

@AgeManning AgeManning added the good first issue Good for newcomers label Mar 18, 2020
@AgeManning
Copy link
Member Author

For this one, I think we can just use igd: https://docs.rs/igd/0.10.0/igd/
I've tested this on my router and seems work fine.
It should be straight forward do something like their example here: https://github.com/sbstp/rust-igd/blob/master/examples/add_port.rs for both the libp2p tcp port and discovery udp port.

I imagine we can run this in it's own thread. We don't need channels or shared memory as I don't think really care about the result (failed or not). If it succeeds, then the ports will be open externally, the local ENR sets the local TCP port regardless, see here: https://github.com/sigp/lighthouse/blob/enr-v0.2.0/beacon_node/eth2-libp2p/src/discovery/enr_helpers.rs#L86
(Note, i'll merge this to v0.2.0 soon).

So if the UPnP is successful, when the node connects to others, discv5 will do it's magic and update the IP to the un-NAT'd external IP and UDP ports.
If it fails, then there was nothing we could do anyway (we could change ports, but I dont think we go down this path just yet).

Therefore, I think it's sufficient to just spawn a thread, that takes a reference to a logger, and attempts to open the ports in the config. It should log failure or success, so at least the user knows.

We should also add a CLI option to disable UPnP support if people like.

I imagine the best place to spawn the thread is somewhere that has access to NetworkConfig. I guess when the network starts up here: https://github.com/sigp/lighthouse/blob/enr-v0.2.0/beacon_node/eth2-libp2p/src/service.rs#L57

Should able to use the info in config to give you all you need to try and open ports on UPnP in a thread from here.

@darcys22
Copy link
Contributor

darcys22 commented Sep 5, 2020

I've had a go at this at #1587

Could I request a review to see if this is on the right track?

In addition, would one make the function asynchronous using the executor parameter that is being passed around the service function?

impl<TSpec: EthSpec> Service<TSpec> {
    pub async fn new(
        executor: environment::TaskExecutor,
        config: &NetworkConfig,
        enr_fork_id: EnrForkId,
        log: &slog::Logger,
    ) 

@ghost ghost added A1 Networking labels Sep 9, 2020
bors bot pushed a commit that referenced this issue Oct 2, 2020
Adding UPnP support will help grow the DHT by allowing NAT traversal for peers with UPnP supported routers.

## Issue Addressed

#927 

## Proposed Changes

Using IGD library: https://docs.rs/igd/0.10.0/igd/

Adding the  the libp2p tcp port and discovery udp port. If this fails it simply logs the attempt and moves on

## Additional Info



Co-authored-by: Age Manning <Age@AgeManning.com>
bors bot pushed a commit that referenced this issue Oct 2, 2020
Adding UPnP support will help grow the DHT by allowing NAT traversal for peers with UPnP supported routers.

## Issue Addressed

#927 

## Proposed Changes

Using IGD library: https://docs.rs/igd/0.10.0/igd/

Adding the  the libp2p tcp port and discovery udp port. If this fails it simply logs the attempt and moves on

## Additional Info



Co-authored-by: Age Manning <Age@AgeManning.com>
@AgeManning
Copy link
Member Author

Resolved in #1587

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

No branches or pull requests

3 participants