Correct /nat API for libp2p
#6677
Conversation
|
Can you rebase this on |
| 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); | ||
| } |
There was a problem hiding this comment.
Nice catch. This is now set in handle_established_inbound_connection instead since #6486.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
jimmygchen
added
waiting-on-author
|
This has been rebased |
AgeManning
force-pushed
the
correct-nat-metrics
branch
from
4b63d9f to
8113d31
Compare
michaelsproul
added
ready-for-review
michaelsproul
force-pushed
the
correct-nat-metrics
branch
from
8113d31 to
6fefd83
Compare
|
Rebased again to remove stuff from |
Squashed commit of the following: commit 6fefd83 Author: Age Manning <[email protected]> Date: Tue Dec 10 16:28:03 2024 +1100 Fix nat API
Issue Addressed
There was a mismatch in the metric naming, resulting in the
/natapi always returning false, when the underlying metric may actually be true.