fixing NAT establish mapping logic

[master255]

If the router's Internet connection is interrupted and then restored, upnp ports are not reopened. With this code, if an error occurs, an attempt is made to retrieve the ip address and all the necessary data to re-announce the upnp ports.
#2502

[Stebalien]

Please flesh out the PR description, explaining how this fixes the issue you encountered.

[master255]

Ready

[Stebalien]

I see. Unfortunately, I don't think this is being fixed in the right place. It would be better to:

1. Move all the logic in

   go-libp2p/p2p/net/nat/nat.go

   Lines 38 to 55 in 44db2b4

   	natInstance, err := discoverGateway(ctx)
   	if err != nil {
   	return nil, err
   	}
   	var extAddr netip.Addr
   	extIP, err := natInstance.GetExternalAddress()
   	if err == nil {
   	extAddr, _ = netip.AddrFromSlice(extIP)
   	}
   	// Log the device addr.
   	addr, err := natInstance.GetDeviceAddress()
   	if err != nil {
   	log.Debug("DiscoverGateway address error:", err)
   	} else {
   	log.Debug("DiscoverGateway address:", addr)
   	}

   into this loop

   go-libp2p/p2p/net/nat/nat.go

   Line 173 in 44db2b4

   for {

2. If/when we fail to establish a mapping, raise an error and re-discover the NAT in that loop.
3. Change

   go-libp2p/p2p/host/basic/natmgr.go

   Line 93 in 44db2b4

   return nmgr.nat != nil

   to ask the NAT if we're actively managing a port mapping.

Otherwise, we'll still fail if, e.g., we fail to detect a NAT on start.

[master255]

@Stebalien Perhaps it is not necessary to try to re-set upnp ports if it failed the first time? After all, if it's 4G, it will only load the CPU.
An application using libp2p should be able to detect network changes and initiate upnp port announcing on its own.
My fix fixes what the application can't detect.

For an example on Android, there is such a notification:

private val networkCallback = object : ConnectivityManager.NetworkCallback() {
    private val networks = mutableListOf<Network>()

    override fun onAvailable(network: Network) {
        super.onAvailable(network)

        networks.add(network)
        Log.d("Has network --->", networks.any().toString())
    }

    override fun onLost(network: Network) {
        super.onLost(network)

        networks.remove(network)
        Log.d("Has network --->", networks.any().toString())
    }
}

val connectivityService =
    applicationContext.getSystemService(Context.CONNECTIVITY_SERVICE) as ConnectivityManager
connectivityService.registerNetworkCallback(networkRequest, networkCallback)

[master255]

We need a new method for the host to force upnp ports to be reopened. But about that in the new pr.

[Stebalien]

I think it's reasonable to provide some external hook to tell libp2p to go "check for new addresses", but IMO, it's not something we should rely on. For example, laptop users will frequently switch networks without such external input.

[master255]

@Stebalien We have to do it this way. That's what all the other systems and known libraries do. For example - https://www.libtorrent.org/single-page-ref.html#reopen-network-sockets. This is a standard.
If we do it your way, it will load the processor.

[Stebalien]

Checking once every few minutes? We already do that when a NAT is available. This is likely the least of our concerns.

[master255]

@Stebalien Once a minute now. Yes. It's a waste of CPU and energy.

I've been using a Libp2p-based music player on my Galaxy S22Ultra phone for about a month now, and I can see that it is only necessary to reduce the Libp2p load. If we increase it, it will already be noticeable for the devices.

[master255]

@Stebalien

For example, laptop users will frequently switch networks without such external input.

My artificial intelligence tells me that Windows has such a callback too :) And I hope the other systems have it too.
This is where we fix the port announcement problem. This is a little bit irrelevant to the problem of changing interfaces.

[marten-seemann]

I've been using a Libp2p-based music player on my Galaxy S22Ultra phone for about a month now, and I can see that it is only necessary to reduce the Libp2p load. If we increase it, it will already be noticeable for the devices.

libp2p in general, and go-libp2p in specific, is not designed to be run on mobile devices. Running a libp2p stack on a phone is not a good idea precisely due to battery considerations (there are further problems that come with the frequent network changes of mobile devices). This is not a use case we are going to optimize for.

[master255]

1. You've already made a library for mobile devices, and I've already written a mobile app based on it.
2. If Libp2p will not be optimized for mobile devices, how will IPFS work on mobile devices? Kubo? Which are based on Libp2p.
3. What if all new devices were mobile?
4. My desktop computer has been a mobile console for a year now. And I don't even have a laptop at home. At the same time, my console has better specs than any of your computers. And it's mobile. Processors are getting smaller and smaller. All my computers are already based on 4 nanometer processors.

Laptops are mobile devices, too.
Maybe you optimize the library for servers only?
Not for clients at all?

My mobile work computer (with external 32 inch monitor):

[Stebalien]

There are many processes in libp2p that run more than once a minute. Even with your patch, NAT detection will continue to run at a regular interval as long as it detects a NAT the first time (e.g., you start your day at home on your home wifi).

The only thing I'm asking here is to apply the same retry logic consistently and I'm not interested in accepting a patch that only half-fixes the problem.

If you need battery savings, the correct solution is to suspend/stop the entire libp2p process when you don't need networking. This once-a-minute NAT check is nothing compared to the regular heartbeats and background chatter go-libp2p will otherwise produce.

[master255]

@Stebalien Your library is very similar in functionality to https://www.libtorrent.org . Why don't you adopt experience of Libtorrent, which has been written and used by billions of people all over the world for a long time?

Stopping Libp2p will cause problems with the DHT. Therefore a temporary shutdown is not possible.

Ok, I'll see what I can do about the loop, but I don't really like creating a load on the CPU for made up reasons.
I know how Libp2p works and have yet to see processes that unnecessarily load the CPU.

[Stebalien]

This isn't a made-up reason, please stop and think about why we're asking for this before continuing to argue.

If you believe libtorrent will provide you a better experience, I suggest you use it.

[master255]

@Stebalien My mobile app has both Libtorrent and Libp2p. And many different, other libraries.
That's the way all mobile apps work.

Yes. I understand you want to avoid the scenario where discoverGateway fails the first time. That's why I said I'd look into it. It's a very rare case, but it's theoretically possible.

[master255]

@Stebalien Good news. In practice, this code is enough. It is not necessary to add new logic.
I tested it for a month with unstable internet on my Media Library application.
If it is necessary to add code here, I will let you know and make a new pr.
If you find new scenarios where the bug is reproduced, then let me know. I will look into them.