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.

Sign up for GitHub

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

Correct /nat API for libp2p #6677

Open
wants to merge 1 commit into
base: release-v6.0.1
Choose a base branch
from
+2 −6
Open

Correct /nat API for libp2p #6677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions beacon_node/lighthouse_network/src/peer_manager/network_behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,6 @@ impl<E: EthSpec> NetworkBehaviour for PeerManager<E> {
debug!(self.log, "Failed to dial peer"; "peer_id"=> ?peer_id, "error" => %ClearDialError(error));
self.on_dial_failure(peer_id);
}
FromSwarm::ExternalAddrConfirmed(_) => {
// We have an external address confirmed, means we are able to do NAT traversal.
metrics::set_gauge_vec(&metrics::NAT_OPEN, &["libp2p"], 1);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch. This is now set in handle_established_inbound_connection instead since #6486.

Copy link
Member

@jxs jxs Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this shouldn't be removed, it tells us when libp2p-upnp was able to successfully map the address and port via UPnP on the router and therefore NAT traversal is activated.
I had missed the code Jimmy linked it, but we receiving an inbound connection doesn't tell us about our NAT traversal settings in libp2p, we may be reached by a peer with a public address that we established a connection before and so the port may still be mapped in the router right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure of when this message actually gets sent.
If this gets sent when upnp worked successfully, then we have some competing conditions about what we say about our libp2p nat being open.

Which is a stronger condition?
UPnP successful or incoming connections?
Both have error cases I think.

  • UPnP could work for a local router, but if behind CGNAT or another router or something, you would not be contactable and not get incoming connections. Maybe we could open the wrong ports also?
  • Incoming connections is fallible as you pointed out ( I think there is a 30s window or something for a peer to disconnect and then re-connect to us. We could increase the number of incoming connections to like 3 or 5 to lower the probability.

To make it simple, I think its easier to reason about with incoming connections. Maybe we have some number of connections (in discovery its 2, i think). If UPnP worked, then I'd expect we would get incoming connections.

The other thing we don't account for is, if after it worked once, we dont check again. Maybe we should also check if we get 0 incoming peers, we should make it as false?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to have some mix if you prefer to keep the UPnP message. Is UPnP the only behaviour that sends this? Could this get sent from other behaviours that might be also fallible?

_ => {
// NOTE: FromSwarm is a non exhaustive enum so updates should be based on release
// notes more than compiler feedback
Expand Down
4 changes: 2 additions & 2 deletions common/system_health/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,14 +235,14 @@ pub fn observe_nat() -> NatState {

let libp2p_ipv4 = lighthouse_network::metrics::get_int_gauge(
&lighthouse_network::metrics::NAT_OPEN,
&["libp2p"],
&["libp2p_ipv4"],
)
.map(|g| g.get() == 1)
.unwrap_or_default();

let libp2p_ipv6 = lighthouse_network::metrics::get_int_gauge(
&lighthouse_network::metrics::NAT_OPEN,
&["libp2p"],
&["libp2p_ipv6"],
)
.map(|g| g.get() == 1)
.unwrap_or_default();
Expand Down
Loading