Nano-node: UPnP leases not created/refreshed ?

[sudokai0]

Summary

UPnP leases are not created / refreshed for me in V22. Conditions for creating a lease seems to be OK. IGD devices are found:
No other UPnP-related logs are generated (I have detailed UPnP-logging enabled)

My analysis for possible root cause:
https://github.com/nanocurrency/nano-node/pull/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: https://github.com/nanocurrency/nano-node/blob/33a974155ddf4b10fc3d2c72e4c20a8abe514aef/nano/node/portmapping.cpp#L127, I think || should be &&. We should skip the refresh if there is an existing lease AND less than half the lease time has passed. Other cases (error case or too much of lease time has passed), we should refresh the lease. But the second part of the if-expression is not evalauted in the success case. In the error case it will be evaluated however, and then "remaining_mapping_duration_l" is likely set to 0 (or remain unmodified of value 0), which would make the second part of the if-expression evaluate to true and the refresh will be skipped.

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

1. Make sure there is an IGD device available in the network
2. Start nano_node
3. Observe in router admin interface / check node log whether a lease has been created

Expected behavior

• UPnP leases created in my router
• Node log indicating that a TCP lease has been created

Actual behavior

• No UPnP leases created in my router
• No node log indicating that a TCP lease has been created

Possible solution

Manually forward needed ports in the router

Supporting files

_No response_

[sudokai0]

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:

[thsfs]

Please check whether these changes makes sense:
https://github.com/nanocurrency/nano-node/compare/develop...upnp_leases_issue?expand=1

The implementation is leading to UPNP_GetSpecificPortMappingEntry failed 714: NoSuchEntryInArray

Captured log:

[DarrenAlex]

I don't see an external IP in your logs. Try adding external_address = "::ffff:99.99.99.99" (replace 99.99.99.99 with your external IP) and external_port = 7075 under the [node] section in config-node.toml. This resolved the issue for me

[sudokai0]

@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 https://github.com/nanocurrency/nano-node/pull/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"): https://github.com/nanocurrency/nano-node/blob/db7540654844ff17e47024a9bf25339bd620c7bb/nano/node/portmapping.cpp#L99 With this fix, there are now seems to be two "competing" timers that refreshes the lease, either "configured lease time - 10 seconds" from refresh_mapping(), or "configured lease time / 2" from check_mappings(), whichever comes first. In the failure case where the router has lost the lease somehow, or failed to add a new lease in refresh_mapping(), the check for remaining lease time doesn't matter.

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?

[dsiganos]

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.

[thsfs]

Thanks for your inputs @sudokai0 , I added a PR for this https://github.com/nanocurrency/nano-node/pull/3298 with some changes, but I'm afraid there is still more work to do, because despite all this implementation, I noticed that the leasing time is not getting refreshed sooner.

[thsfs]

I don't see an external IP in your logs. Try adding external_address = "::ffff:99.99.99.99" (replace 99.99.99.99 with your external IP) and external_port = 7075 under the [node] section in config-node.toml. This resolved the issue for me

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.

[thsfs]

Code already merged (#3298)