remove nat module and use libp2p upnp #4840

jxs · 2023-10-12T17:33:16Z

Issue Addressed

uses the new libp2p upnp module to map internal ports to the external network instead of the local nat module.

Additional Info

This is a first move on fulfilling #4816 as we can just use libp2p to understand if ports are publicly accessible and it will act accordingly with the FromSwarm::ExternalAdrConfirmed.
There's a downside though, we lose the ability to map the DiscV5 udp port, for that I suggest we implement it as a feature in Discovery itself wdyt? I can submit a PR for that

divagant-martian · 2023-10-12T17:48:35Z

can't libp2p-upnp be extended to track arbitrary user ports? adding to discovery is something I would prefer not to do in part to avoid the double underlying upnp dependency, and the fact that it's simply duplicated code.

Making libp2p-upnp handle this I don't think is egregious. It's not too different to allow-block-list handling bans and allows for peers libp2p is unaware for example. It could simply need to be able to report that the external address is not one from the libp2p ports, but an user managed one

AgeManning · 2023-10-17T00:50:21Z

ahh yeah good point. When we were discussing this I didn't consider discovery.

My opinion is that the code in lh for UPnP isn't great. It was written a while ago and I consider it to always fail (i.e dont rely on it in any way).

If libp2p is going to maintain its own version as a dep I think it kind of makes sense to export it there. Perhaps we want to do a similar thing in discovery. I agree that its ugly that we duplicate the dependencies and then attempt UPnP twice this way, but the benefit is that its then logically grouped into each transport that needs it and I guess discovery users independently can benefit from its own internal UPnP functionality.

The overhead of running it twice on boot prob isn't that bad. What do you think @divagant-martian?

jxs · 2023-10-18T17:23:29Z

Hi Diva, I don't think allow-block-list is comparable as it works by listening on handle_established_{inbound_outbound}_connection methods which happen after libp2p's noise handshake and check for PeerId's one has banned/allowed, PeerIds are a libp2p property and exist after the noise handshake.

I also don't think it doesn't make sense to invent a new multiaddress for this.
But I can understand though that having UPnP in Discovery might be out of scope, so I refactored the nat module with some changes as spoken with @AgeManning:

make it async and use the same dep as libp2p to avoid multiple igd deps
use timed leases so we don't need Drop logic
use the listening address instead of using if-watch to search for interfaces

divagant-martian · 2023-10-18T17:56:57Z

thanks @jxs ot sure what you mean about creating a new multi address? in any case, this is what I discussed and proposed to @AgeManning a couple days ago and what we decided would be best, ty!

AgeManning

This looks good.

However there is a CLI flag --disable-upnp. I think we still need to respect that and therefore not use libp2p upnp or discovery upnp when that flag is set.

AgeManning · 2023-10-19T03:39:36Z

beacon_node/network/src/nat.rs

+            )
+            .await
+            .context("Could not UPnP map port: {port} on the gateway")?;
+        info!(log, "Discovery UPnP port mapped"; "port" => %port);


We have a few info logs at the moment.
We have one from creating in libp2p, one from creating discovery and then the results from both.

I think its probably safe to assume if libp2p works, then discovery probably will to. Maybe we make the discovery logs debug?

yup, makes sense Age. Updated!

reduce logs to debug!

…bp2p-nat

mxinden

Great to see libp2p-upnp in action soon.

ackintosh

This looks good to me, but I left some minor comments.

ackintosh · 2023-11-04T23:06:23Z

beacon_node/network/src/service.rs

-                    "UPnP",
-                );
-            }
+        if let (true, Some(v4)) = (config.upnp_enabled, config.listen_addrs().v4()) {


I think we need to respect config.disable_discovery too.

yeah makes sense, updated!

ackintosh · 2023-11-04T23:17:07Z

beacon_node/network/src/nat.rs

+                "Lighthouse Discovery port",
+            )
+            .await
+            .context("Could not UPnP map port: {port} on the gateway")?;


It seems that the context() doesn't parse the {port} placeholder correctly.

Here is a log when I run beacon node with branch:

Nov 04 23:05:30.569 INFO Could not UPnP map Discovery port, error: Could not UPnP map port: {port} on the gateway, module: network::service:240

thanks for trying this and providing feedback, updated!

ackintosh · 2023-11-05T00:41:27Z

beacon_node/lighthouse_network/src/service/mod.rs

@@ -1516,6 +1517,47 @@ impl<AppReqId: ReqId, TSpec: EthSpec> Network<AppReqId, TSpec> {
        }
    }

+    fn parse_upnp_event(&mut self, event: libp2p_upnp::Event) {


Since the existing methods for handling events are named with the prefix inject_, perhaps we should align with this naming convention? For example, we could name it inject_upnp_event.

yeah makes sense. Updated, thanks!

…bp2p-nat

jxs

Thanks for the review Akihito!

jxs · 2024-01-22T14:04:33Z

beacon_node/lighthouse_network/src/service/mod.rs

@@ -1516,6 +1517,47 @@ impl<AppReqId: ReqId, TSpec: EthSpec> Network<AppReqId, TSpec> {
        }
    }

+    fn parse_upnp_event(&mut self, event: libp2p_upnp::Event) {


yeah makes sense. Updated, thanks!

jxs · 2024-01-22T14:06:46Z

beacon_node/network/src/nat.rs

+                "Lighthouse Discovery port",
+            )
+            .await
+            .context("Could not UPnP map port: {port} on the gateway")?;


thanks for trying this and providing feedback, updated!

jxs · 2024-01-22T14:06:59Z

beacon_node/network/src/service.rs

-                    "UPnP",
-                );
-            }
+        if let (true, Some(v4)) = (config.upnp_enabled, config.listen_addrs().v4()) {


yeah makes sense, updated!

…bp2p-nat

beacon_node/lighthouse_network/Cargo.toml

ackintosh · 2024-02-19T06:34:51Z

beacon_node/network/src/service.rs

+        ) {
+            let nw = network_log.clone();
+            let v4 = v4.clone();
+            tokio::spawn(async move {


Should we use executor.spawn to spawn a future here, since we were using executor.spawn_blocking before?

yeah, thanks for the catch Akihito! Updated

…bp2p-nat

ackintosh

Thanks for the update! Looks good to me.

jxs · 2024-02-22T08:24:13Z

@Mergifyio queue

mergify · 2024-02-22T08:24:36Z

queue

🟠 Waiting for conditions to match

any of: [🔀 queue conditions]
- all of: [📌 queue conditions of queue default]
  - #approved-reviews-by >= 1
  - check-success=license/cla
  - check-success=target-branch-check
-draft [📌 queue requirement]
any of: [📌 queue -> configuration change requirements]
- -mergify-configuration-changed
- check-success=Configuration changed

AgeManning · 2024-02-27T05:00:07Z

@Mergifyio queue

mergify · 2024-02-27T05:00:32Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at abd9965

* remove nat module and use libp2p upnp * update Cargo.lock * remove no longer used dependencies * restore nat module refactored * log successful mapping * only activate upnp if config enabled reduce logs to debug! * Merge branch 'unstable' of https://github.com/sigp/lighthouse into libp2p-nat * Merge branch 'unstable' of https://github.com/sigp/lighthouse into libp2p-nat * Merge branch 'unstable' of https://github.com/sigp/lighthouse into libp2p-nat * address review * Merge branch 'unstable' of https://github.com/sigp/lighthouse into libp2p-nat * Merge branch 'unstable' of https://github.com/sigp/lighthouse into libp2p-nat * Merge branch 'unstable' of https://github.com/sigp/lighthouse into libp2p-nat * address review

jxs added 4 commits October 18, 2023 18:23

remove nat module and use libp2p upnp

0a6d3d2

update Cargo.lock

706902a

remove no longer used dependencies

ea221db

restore nat module refactored

0e7ed19

jxs force-pushed the libp2p-nat branch from 94977a2 to 0e7ed19 Compare October 18, 2023 17:25

log successful mapping

a459b75

AgeManning reviewed Oct 19, 2023

View reviewed changes

jxs added 3 commits October 19, 2023 10:23

only activate upnp if config enabled

2948d75

reduce logs to debug!

Merge branch 'unstable' of https://github.com/sigp/lighthouse into li…

7c6c40e

…bp2p-nat

Merge branch 'unstable' of https://github.com/sigp/lighthouse into li…

8e1fd07

…bp2p-nat

mxinden reviewed Oct 20, 2023

View reviewed changes

ackintosh reviewed Nov 5, 2023

View reviewed changes

ackintosh mentioned this pull request Jan 7, 2024

Another UPnP/igd panic #5020

Closed

dapplion added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Jan 19, 2024

Merge branch 'unstable' of https://github.com/sigp/lighthouse into li…

76d3963

…bp2p-nat

jxs commented Jan 22, 2024

View reviewed changes

jxs force-pushed the libp2p-nat branch from adf2796 to bd78c82 Compare January 22, 2024 18:39

address review

0758bf3

jxs force-pushed the libp2p-nat branch from bd78c82 to 0758bf3 Compare January 22, 2024 18:40

jxs added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 8, 2024

jxs added 2 commits February 8, 2024 15:01

Merge branch 'unstable' of https://github.com/sigp/lighthouse into li…

455eb75

…bp2p-nat

Merge branch 'unstable' of https://github.com/sigp/lighthouse into li…

914ae8b

…bp2p-nat

jxs requested a review from ackintosh February 12, 2024 14:47

AgeManning reviewed Feb 18, 2024

View reviewed changes

beacon_node/lighthouse_network/Cargo.toml Show resolved Hide resolved

AgeManning removed the ready-for-review The code is ready for review label Feb 18, 2024

AgeManning added the waiting-on-author The reviewer has suggested changes and awaits thier implementation. label Feb 18, 2024

ackintosh reviewed Feb 19, 2024

View reviewed changes

jxs added 2 commits February 19, 2024 11:02

Merge branch 'unstable' of https://github.com/sigp/lighthouse into li…

0fa03b7

…bp2p-nat

address review

3c229f3

jxs requested a review from ackintosh February 19, 2024 11:21

ackintosh approved these changes Feb 22, 2024

View reviewed changes

AgeManning approved these changes Feb 27, 2024

View reviewed changes

mergify bot added a commit that referenced this pull request Feb 27, 2024

Merge of #4840

5f198f7

mergify bot mentioned this pull request Feb 27, 2024

merge queue: embarking unstable (d36241b) and #4840 together #5317

Closed

5 tasks

mergify bot merged commit abd9965 into sigp:unstable Feb 27, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove nat module and use libp2p upnp #4840

remove nat module and use libp2p upnp #4840

jxs commented Oct 12, 2023 •

edited

Loading

divagant-martian commented Oct 12, 2023 •

edited

Loading

AgeManning commented Oct 17, 2023

jxs commented Oct 18, 2023 •

edited

Loading

divagant-martian commented Oct 18, 2023 •

edited

Loading

AgeManning left a comment

AgeManning Oct 19, 2023

jxs Oct 20, 2023

mxinden left a comment

ackintosh left a comment

ackintosh Nov 4, 2023

jxs Jan 22, 2024

ackintosh Nov 4, 2023

jxs Jan 22, 2024

ackintosh Nov 5, 2023

jxs Jan 22, 2024

jxs left a comment

jxs Jan 22, 2024

jxs Jan 22, 2024

jxs Jan 22, 2024

ackintosh Feb 19, 2024

jxs Feb 19, 2024

ackintosh left a comment

jxs commented Feb 22, 2024

mergify bot commented Feb 22, 2024

AgeManning commented Feb 27, 2024

mergify bot commented Feb 27, 2024 •

edited

Loading

remove nat module and use libp2p upnp #4840

remove nat module and use libp2p upnp #4840

Conversation

jxs commented Oct 12, 2023 • edited Loading

Issue Addressed

Additional Info

divagant-martian commented Oct 12, 2023 • edited Loading

AgeManning commented Oct 17, 2023

jxs commented Oct 18, 2023 • edited Loading

divagant-martian commented Oct 18, 2023 • edited Loading

AgeManning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

ackintosh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackintosh left a comment

Choose a reason for hiding this comment

jxs commented Feb 22, 2024

mergify bot commented Feb 22, 2024

🟠 Waiting for conditions to match

AgeManning commented Feb 27, 2024

mergify bot commented Feb 27, 2024 • edited Loading

✅ The pull request has been merged automatically

jxs commented Oct 12, 2023 •

edited

Loading

divagant-martian commented Oct 12, 2023 •

edited

Loading

jxs commented Oct 18, 2023 •

edited

Loading

divagant-martian commented Oct 18, 2023 •

edited

Loading

mergify bot commented Feb 27, 2024 •

edited

Loading