UPnP leases not created/refreshed ? #3295
Comments
|
I was able to confirm that UPnP leases are created properly if I remove the second part of the if-expression, and running a recompiled node. Then I can see the following (new entry) in the log: [2021-May-18 17:31:37.666290]: UPnP TCP port mapping response: 0 |
|
Please check whether these changes makes sense: The implementation is leading to UPNP_GetSpecificPortMappingEntry failed 714: NoSuchEntryInArray Captured log: |
|
I don't see an external IP in your logs. Try adding |
|
@thsfs : I think that the changes makes sense, in that will enable leases at startup to be created again. But I wonder, won't the scheduled refresh created by refresh_mapping() already handle to renew leases? I wonder a bit what the original purpose of adding the remaining lease time check in check_mapping was (in #3035)? Before #3035, check_mapping() was responsible for triggering the initial lease to be created, by detecting that there was no existing lease. Once a lease has been setup however, refresh_mapping() will schedule to refresh it on it's own (with a timer of "configured lease time - 10 seconds"): nano-node/nano/node/portmapping.cpp Line 99 in db75406 I don't know the full history of this implementation , and my analysis of the source code might miss something, but maybe worth to check this a bit more? |
|
Hi @sudokai0, your analysis makes sense to me. I think refresh_mapping spawned from inside refresh_mapping should not be needed since there is an outer loop checking regularly. We just have to make sure that the outer loop checks at least twice as fast as the lease time. Currently it checks every 5 minutes irrespective of the lease time. |
the port is different (17075) because I ran it on a different network. The external IP address is this "187.20.X.Y" with the end changed to X.Y. |
|
Code already merged (#3298) |
Summary
UPnP leases are not created / refreshed for me in V22. Conditions for creating a lease seems to be OK. IGD devices are found:
[2021-May-17 21:15:42.807454]: UPnP local address: 192.168.1.230, discovery: 0, IGD search: 1
[2021-May-17 21:15:42.807454]: UPnP device url: http://192.168.1.251:39446/rootDesc.xml st: urn:schemas-upnp-org:device:InternetGatewayDevice:1 usn: uuid:ef526755-528c-4513-b89c-cdc8891c669d::urn:schemas-upnp-org:device:InternetGatewayDevice:1
[2021-May-17 21:15:42.807454]: UPnP device url: http://192.168.1.1:5000/rootDesc.xml st: urn:schemas-upnp-org:device:InternetGatewayDevice:1 usn: uuid:ea29f574-55d2-493c-aa12-4f8c5bbed04c::urn:schemas-upnp-org:device:InternetGatewayDevice:1
[2021-May-17 21:15:42.822414]: UPnP TCP mapping verification response: 714, external ip response: 0, external ip: , internal ip: 192.168.1.230, lease: <- this line repeats periodically, but the lease value is never set to anything
No other UPnP-related logs are generated (I have detailed UPnP-logging enabled)
My analysis for possible root cause:
#3035 was merged to fix refreshing of "lost" UPnP leases. As I understand it, the intention is to trigger a refresh of a lease when more than half of the lease time has passed, instead of when it had already expired.
On this line:
nano-node/nano/node/portmapping.cpp
Line 127 in 33a9741
If the above analysis is correct, then initial leases will never be triggered to be created, since check_mapping() won't trigger it to be added (due to the ||-logic described above), and nothing else seems to trigger it either. Could be verified by never seeing "UPnP %1%:%2% mapped to %3%") % protocol.external_address % config_port_l % node_port_l));" (with values for each % part) in the node log when the node starts up. I tried it on my local node, and I didn't see it in my log file at least.
Even if creating of initial leases is fixed, then another problem could still occur if a lease is "lost" and the fix doesn't take it into account. These are normally detected by calling check_mapping() periodically => lease will still be renewed though to due to refresh_mapping() having scheduled itself to renew created leases. If refresh_mapping() fails to add a lease (at startup or when renewing an existing one) at some point, it will never be scheduled to be added again since check_mapping() will not trigger it either (due to using the "||"-logic described above). If the log on line 106 is ever seen in the log file "UPnP failed ...", then I think the lease could be lost until the process is restarted.
Node version
V22
Build details
Official binaries
OS and version
Windows 10
Steps to reproduce the behavior
Expected behavior
Actual behavior
Possible solution
Manually forward needed ports in the router
Supporting files
No response
The text was updated successfully, but these errors were encountered: