net: Replace libnatpmp with built-in PCP+NATPMP implementation #30043

Continues #30005. Closes #17012..

This PR adds PCP (Port Control Protocol) from RFC6887. This adds, in addition to the existing IPv4 port mapping (which now uses PCP, with fallback to NAT-PMP), support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.

PCP, like NAT-PMP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should otherwise be a drop-in replacement. NAT-PMP fallback is implemented so this will not make router support worse.

For now it is disabled by default, though in the future (not in this PR) we could consider enable it by default to increase the number of connectable nodes without adding significant attack surface.

To test:

0bitcoind -regtest -natpmp=1 -debug=net

(most of the changes in this PR are, ironically, removing the libnatpmp dependency and associated build system and build docs)

TODO

• Default gateway discovery on Linux / FreeBSD
• Default gateway discovery on Windows
• Default gateway discovery on MacOS
• Either solve FreeBSD compile issue (probably upstream issue) or remove FreeBSD support
• (maybe) split src/util/netif.h off into a separate PR #30043#pullrequestreview-2047432934

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #30277 ([DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation) by sr-gi)
• #29852 ([WIP] build: remove need to test for endianness by fanquake)
• #29790 ([DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system – WIP by hebasto)
• #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
• #28710 (Remove the legacy wallet and BDB dependency by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24606743714

Whoa :)

(Concept ACK)

RFC6887 appendix A describes how a router that supports NAP-PMP but not PCP, will return UNSUPP_VERSION. A log message could encourage users to try -upnp instead (if not already enabled). Or upgrade their ancient router firmware :-)

Got distracted during review, will continue later. I’ll look into how we can preserve a previously selected NatPMP checkbox in the GUI.

Regarding the custom lib/self-impl discussion in #30005: Taking a quick look at the implementation here, I think it’s simple enough for us to maintain ourselves. If a nice canonical lib ever emerges we could always jump to it, as there are only a few basic functions and presumably we could probably switch them out close to 1:1.

That said, it is a little rough to review as

• It needs to be compared to the letter of the spec, with the assumption that @laanwj is evil or has mis-implemented (I doubt that :)
• It needs to be very defensive, though the attack surface seems quite minimal
• It needs to be aware of real-world violators/benders/extenders of the spec (if any? I have no idea.)

But it seems worth the effort to me.

That said, it is a little rough to review as It needs to be compared to the letter of the spec, with the assumption that @laanwj is evil or has mis-implemented (I doubt that :)

If you’re more comfortable comparing it against another implementation there’s:

• A kinda haphazard windows implementation here: https://github.com/moonlight-stream/GS-IPv6-Forwarder/blob/master/GSv6Fwd/pcp.cpp
• Miniupnpd’s pcp server implementation https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/pcpserver.c

It needs to be aware of real-world violators/benders/extenders of the spec (if any? I have no idea.)

Miniupnpd’s is, likely, the most common server implementation in the wild. It’s been tested against that. i’m hoping people will test this on various routers and conditions, there will be inevitable edge cases to iron out.

If a nice canonical lib ever emerges we could always jump to it

Maybe, but at some point there’s not much difference between implementing a simple request/reply protocol and using a library. At least the RFC is unambigiously documented, which can’t be said of many FOSS ABI’s. Also, @fanquake (and @gmaxwell in the past) has been hoping for a solution that doesn’t introduce a library dependency.

Would prefer this in two steps (add PCP, then remove NAT-PMP)

i’m not planning to do this, the build system commits are already set up this way, but doing it throughout would involve adding another setting in Qt just to remove it later. Same for adding a third mechanism in portmap.cpp. Agree with @sjors that having a forest of different port mapping settings is confusing to the user.

Edit: no longer relevant now that this implements PCP with NAT-PMP fallaback.

Tested on Ubuntu 24.04 I’m getting in bound via IPv4 and IPv6.

When a GUI user previously had NAT-PMP selected there would be a "natpmp": true field in settings.json.

We need to rename this to "tcp" in order for the box to remain checked.

Maybe @ryanofsky has an idea how to do this elegantly?

A little hack that works is to put the following at the end of ReadSettings() in settings.cpp:

0    // Migrate settings:
1    if (values.contains("natpmp")) {
2        auto el = values.extract("natpmp");
3        el.key() = "pcp";
4        values.insert(std::move(el));
5    }

A bit nicer would be to have a MigrateSettingsFile() that’s called between ReadSettingsFile and WriteSettingsFile in init.cpp.

(not to be confused with OptionsModel::checkAndMigrate() which is called before settings.json is read)

Maybe you can split src/util/netif.h off into a separate PR, along with b06edec229d5cfc8d0d1a19bd723852e6bcfd9d9.

In terms of which IPv6 address to use, my understanding is this:

1. In the olden days the IPv6 address contained part of the MAC address. This ensured uniqueness, but was bad for mobile device privacy; no matter where you went, part of your IP address was constant. Such addresses always (?) contain 0xfffe. See https://datatracker.ietf.org/doc/html/rfc7707
2. A new standard was introduced that uses a hash of both the MAC address and the prefix, ensuring it’s stable if you stay on the same network, but changes when you move. See https://datatracker.ietf.org/doc/html/rfc7217#page-19
3. There’s also temporary addresses, which are rotated every couple of hours. See https://datatracker.ietf.org/doc/html/rfc8981

If we were able to tell which one one is which, then I think we should pick only one, in order of preference: (2), (3), (1). This reflects our desire to actually get inbound connections, even after a shutdown, while at the same time not doxxing ourselves when on the move (mainly for laptops, perhaps mobile in the future).

We can easily detect type (1) by looking for 0xfffe at the right position (and then least prefer it).

I believe you can detect (2) by looking for IFA_F_STABLE_PRIVACY in flags of the inet6_ifaddr struct. It seems getifaddrs doesn’t have access to that. Neither does /proc/net/if_inet6 since https://patchwork.ozlabs.org/project/netdev/patch/1386680189-7852-1-git-send-email-jiri@resnulli.us/

Afaik there’s also no guarantee about the order of results.

So maybe we should just pick one at random rather than announce all three. This also seems orthogonal to PCP.

Regarding gateway discovery on macOS and Windows, the NatPMP C code is probably as good a place to start as any: https://github.com/miniupnp/libnatpmp/blob/master/getgateway.c

I tested that -natpmp works (before this PR) on macOS 14.4.1.

You already implemented the USE_PROC_NET_ROUTE approach, which unfortunately is the shortest of them.

Here’s another one: https://github.com/libpcp/pcp/blob/master/libpcp/src/net/gateway.c

Life would be so much easier if RFC6887 had allowed you to send the request to some random destination on the internet and just have the router intercept it (with Hop Limit 1 for privacy).

One creative cross-platform way to find the default gateway is, like traceroute, to send an ICMP(v6) message towards a random IPv4/6 address (via TCP), with Hop Limit set to 1, wait for the Destination Unreachable Message response and get the origin IP. https://datatracker.ietf.org/doc/html/rfc792

If PCP servers announced themselves via DNS-Based Service Discovery rfc6763 that would also make things easier. But waiting for an amendment of / followup to rfc792 and all routers to implement it … would take a while. And then you need support that protocol (mostly fetching and parsing DNS TXT records).

Thanks!

On Windows (Vista and higher, which is all we care about) getting the default gateway is straightforward, there’s GetBestRoute2 that does all the work. It’s part of netioapi which we already need for interface enumeration. MacOS’s sysctl is a bit uglier.

If PCP servers announced themselves via DNS-Based Service Discovery rfc6763 that would also make things easier.

i don’t think it really would, we’d need some special library for DNS access, getting TXT records isn’t built into libc.

One creative cross-platform way to find the default gateway is, like traceroute, to send an ICMP(v6) message towards a random IPv4/6 address (via TCP), with Hop Limit set to 1, wait for the Destination Unreachable Message response and get the origin IP.

It’s a clever idea, but sending/receiving ICMP needs raw socket privileges on many operating systems.

Maybe you can split src/util/netif.h off into a separate PR, along with https://github.com/bitcoin/bitcoin/commit/b06edec229d5cfc8d0d1a19bd723852e6bcfd9d9.

Sure, could do that.

So maybe we should just pick one at random rather than announce all three. This also seems orthogonal to PCP.

Thanks, that’s interesting.

One problem is that routers might fail to pinhole some of the addresses. For example, Fritz!Box refuses to open ports on temporary privacy ones, which aren’t a distinguishable range or type (on purpose). Might end up having no open port at all, or only for a temporary address lifetime. So for this PR, i prefer to keep the “announce all publicly routable” that is the current Discover() behavior for now.

Fine with discussing changes to IPv6 address choosing behavior later, as you say, it’s orthogonal to adding PCP.

• Rebased for a trivial merge conflict related to clang installation in .github/workflows/ci.yml.
• Needed to change the use of bitcoin_config.h to make the lint pass.
• Split netif.h and netif.cpp creation into a seperate commit (could be a seperate PR later).
• Addressed a few comments.

Replaced Linux-specific QueryDefaultGateway with @vasild’s netlink implementation for Linux and FreeBSD. This may even generalize to more UNIX variants.

Will tackle Windows next.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25069227736

Added a (mostly untested for now) Windows implementation of QueryDefaultGateway, if someone could test this it’d be very helpful. i will try in WINE later.

Edit: Wine has only a stub for GetBestRoute2, so tested on Amazon EC2 actual windows, can’t check PCP due to lack of a suitable router there, but IP address and default gateway finding works:

02024-05-17T14:17:48Z [net] pcp: gateway [IPv4]: 172.31.16.1
12024-05-17T14:17:48Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 18444 from gateway 172.31.16.1                                                                                                                                   
22024-05-17T14:17:48Z [net] pcp: Internal address after connect: 172.31.28.xxx                                                                             
32024-05-17T14:17:51Z [net] pcp: gateway [IPv6]: fe80::58:f4ff:feca:xxxx%7
42024-05-17T14:17:51Z [net] pcp: Requesting port mapping for addr 2600:1f1c:b80:8af0:e073:32d6:80b3:xxxx port 18444 from gateway fe80::58:f4ff:feca:xxxx%7
52024-05-17T14:17:51Z [net] pcp: Internal address after connect: 2600:1f1c:b80:8af0:e073:32d6:80b3:xxxx

Do we have any information about real-world deployment of PCP vs NAT-PMP? I see PCP dates from 2013, but do we know if it was adopted immediately (and/or, how many older routing devices are in common use)?

RFC 6887 Appendix A explains how one can be compatible with both, though I don’t know how much work it would be to implement.

i have no idea, also no idea how to get that information; to be honest if we can remotely avoid it, i’d prefer not to implement unnecessary compatibility stuff, with the added complexity that entails, this is already a lot to ask people to review as-is.

Edit: but this is what we did.

Yeah, fair enough. Absent any reports of “doesn’t work on my router while nat-pmp worked” (which we’re quite unlikely to get) there is really no way to assess that.

For what it’s worth, initial support for PCP was added in miniupnpd in 2013. So the implementation wasn’t lagging much behind the standard. Of course, not all routers use miniupnpd but it’s extremely common.

If we get reports like that, we can decide to re-add NAT-PMP in a similar way as the current code., without reintroducing the dependency. It’s not that different, it can share the gateway finding code, for one.

Trying this with -pcp=1 on my home internet connection:

 02024-05-20T23:28:41.625432Z [net] pcp: gateway [IPv4]: 192.168.1.1
 12024-05-20T23:28:41.625469Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 8333 from gateway 192.168.1.1
 22024-05-20T23:28:41.625529Z [net] pcp: Internal address after connect: 192.168.1.254
 32024-05-20T23:28:41.626388Z [net] pcp: Received response of 8 bytes: 008100010046cfb7
 42024-05-20T23:28:41.626404Z [net:warning] pcp: Response too small
 52024-05-20T23:28:42.627214Z [net] pcp: Timeout
 62024-05-20T23:28:42.627258Z [net] pcp: Retrying (1)
 72024-05-20T23:28:42.627870Z [net] pcp: Received response of 8 bytes: 008100010046cfb8
 82024-05-20T23:28:42.627901Z [net:warning] pcp: Response too small
 92024-05-20T23:28:43.628167Z [net] pcp: Timeout
102024-05-20T23:28:43.628228Z [net] pcp: Retrying (2)
112024-05-20T23:28:43.628976Z [net] pcp: Received response of 8 bytes: 008100010046cfb9
122024-05-20T23:28:43.628999Z [net:warning] pcp: Response too small
132024-05-20T23:28:44.629173Z [net] pcp: Timeout
142024-05-20T23:28:44.629225Z [net] pcp: Giving up after 3 tries
152024-05-20T23:28:44.632097Z [net] pcp: Could not determine IPv6 default gateway

Trying this with -pcp=1 on my home internet connection:

Thanks for testing! The received packet is not a valid PCP response (too short). But interpreting it as one anyway, the version byte is 0x00, result code is 0x01 (UNSUPP_VERSION). So you might have one of those routers that supports NAT-PMP only.

@laanwj This is a router provided by Verizon (a large US ISP) in 2021.

That’s curious, especially as NAT-PMP doesn’t have any IPv6 support. i’m assuming you don’t have any output for IPv6?

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]

The only TODO that is left to do is the Qt settings migration

Avoiding this complexity is another reason to add a NAT-PMP fallback, then we’d be right to keep the option named the same and only change the descriptions.

Edit: done

Avoiding this complexity is another reason to add a NAT-PMP fallback, then we’d be right to keep the option named the same and only change the descriptions.

If you think such a fallback is easier than dealing with QT settings migration :-)

We can just sort them PCP, NAT-PMP, UPNP in the GUI. If we also add the word “recommended” to PCP, it should mitigate choice-paralysis. Then in the future we can add a little green dot to indicate that it actually works.

I tested e6656f981019331db869e5bed1fcb5247bafbc3f on Windows , (Intel) macOS and Linux.

On macOS 14.5 it opens IPv4 and IPv6 ports, which eventually leads to inbound connections (I used a non-8333 port so it takes a while).

The OPNsense status page only shows IPv4 mappings, not IPv6 hole punch, but the latter clearly works too.

On Windows IPv4 mapping works:

02024-05-22T11:54:07Z [net:info] pcp: Mapping successful: we got x.x.x.x:x for 1260 seconds.
12024-05-22T11:54:07Z [net:info] pcp: ExternalIPv4Address:port = x.x.x.x:x
22024-05-22T11:54:07Z AddLocal(x.x.x.x:x,3)

And it gets plenty of inbound IPv4 connections.

IPv6 mapping failed - I think; the message itself isn’t specific that this was IPv6:

02024-05-22T11:54:07Z [net:warning] pcp: Mapping failed with result NO_RESOURCES (code 8)
12024-05-22T11:54:07Z [net:warning] pcp: Mapping failed with result NO_RESOURCES (code 8)
22024-05-22T11:54:08Z [net:warning] pcp: Mapping failed with result NO_RESOURCES (code 8)

When I restarted the node I noticed that it didn’t even try for IPv6 and that the AddLocal / Discover messages for IPv6 addresses happened after the IPv4 mapping was successful. So maybe it needs to wait a bit? But then upon a third restart I got the same sequence of events, but this time it gave a NO_RESOURCES error again (with IPv4 working fine).

Oh and now I’m getting NO_RESOURCES on my Mac as well, so I guess I just upset the router.

If you think such a fallback is easier than dealing with QT settings migration :-)

Yes. It also avoids having to rename the command-line option. It would make the new code a drop-in replacement that also supports IPv6, no change for users.

It’s a matter of handling UNSUPP_VERSION packets (the one @sipa gets), then re-trying through NAT-PMP, for IPv4. i’m already halfway implementing it.

(see https://datatracker.ietf.org/doc/html/rfc6886#section-3.5 and https://datatracker.ietf.org/doc/html/rfc6887#appendix-A)

Restarting the node with the same port seems to trigger the NO_RESOURCES error with IPv6. I guess that’s because the previous “lease” is still valid. It worked fine on Windows once I picked a fresh port.

During a clean shutdown, we should probably ask the gateway to delete the mapping :

0Requested lifetime (in common header):  Requested lifetime of this
1      mapping, in seconds.  The value 0 indicates "delete".

Update: despite the NO_RESOURCES I still get inbound IPv6 connections, so we can also just ignore it, but then we don’t know when to renew.

During a clean shutdown, we should probably ask the gateway to delete the mapping :

i wasn’t sure about this! Yes, it would be cleaner, but also complicates the code (it would have to keep track of current mappings state). In normal use it’s unlikely for a user to restart a node in such a small timespan, and having a node unconnectable for a few minutes isn’t a big deal. The mappings aren’t too long (20 min) and expire automatically, and it will re-try periodically until it gets one. So it’s a problem that solves itself.

If we auto-renew every 20 minutes then I guess that’s fine. Maybe just add a comment somewhere that we could explicitly delete the mapping upon shutdown.

As well as clarifying the log message for NO_RESOURCES with something like “This is expected after a restart and should clear after N minutes.”.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25289226001

Success! My node is reachable publicly, without configuration.

 02024-05-23T02:06:39.536932Z [net] pcp: gateway [IPv4]: 192.168.1.1
 12024-05-23T02:06:39.536973Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 8333 from gateway 192.168.1.1
 22024-05-23T02:06:39.537027Z [net] pcp: Internal address after connect: 192.168.1.254
 3...
 42024-05-23T02:06:39.538066Z [net] pcp: Received response of 8 bytes: [...]
 52024-05-23T02:06:39.538101Z [net] pcp: Got unsupported version response, falling back to NAT-PMP
 62024-05-23T02:06:39.538118Z [net] natpmp: Requesting port mapping port 8333 from gateway 192.168.1.1
 72024-05-23T02:06:39.539006Z [net] natpmp: Received response of 12 bytes: [...]
 8...
 92024-05-23T02:06:39.551712Z [net] natpmp: Received response of 16 bytes: [...]
102024-05-23T02:06:39.551736Z [net:info] natpmp: Mapping successful: we got [...] for 1200 seconds.
112024-05-23T02:06:39.551774Z [net:info] natpmp: ExternalIPv4Address:port = [...]
122024-05-23T02:06:39.551787Z AddLocal([...],3)
132024-05-23T02:06:39.551826Z [net] pcp: Could not determine IPv6 default gateway

(IP address scrubbed)

Success! My node is reachable publicly, without configuration.

Nice!!! Thanks for testing again.

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/25337033465

Ok, happy with QueryDefaultGateway(Impl) as of 9a265c6d75136991125730b8a6a901e95cfeb8f6, except for not compiling on BSD 13.2:

0util/netif.cpp:84:82: error: use of undeclared identifier 'typeof'; did you mean 'typeid'?
1    for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {

It also throws a warning:

0util/netif.cpp:84:47: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
1    for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {

FreeBSD clang version 14.0.5

So I guess that’s not supposed to work, since clang 15.0 is the minimum.

0pkg install llvm15
1./configure CC=/usr/local/bin/clang15 CXX=/usr/local/bin/clang++15 MAKE=gmake
2gmake

That still results in the same error and warning though.

Do you know what’s the minimum FreeBSD version it can be compiled on? Let’s bump the version bound to that.

util/netif.cpp:84:82: error: use of undeclared identifier ’typeof’; did you mean ’typeid’? for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {

It is strange though. There isn’t even a “typeof” on that line. It must be part of the system macro itself? In which case, why do they provide macros that don’t work with their own compilers.

Edit: googling for the issue, typeof is a GCC specific extension that’s not available in C++. It should be using __typeof__ instead. Unfortunately, it is not in our own code.

Do you know what’s the minimum FreeBSD version it can be compiled on? Let’s bump the version bound to that.

I’ll spin up some more recent VMs to test, probably next week though.

pcp.{h,cpp} (mostly) look good to me as well, but I haven’t tested the NAT-PMP fallback. As do the changes to mapport.{h,cpp}.

Next on my review list is the remaining code that drops the old Nat-PMP dependency, but that’s all a lot simpler.

The first two commits of #26812 would make it possible to test and fuzz how this code interacts with a router.

So ive been thinking about this, do we have a mockable way to do std::optional<Sock> socket(int domain, int type, int protocol)? i was thinking of passing in a SocketFactory (please don’t kill me, this could be just a functor 😅 ). It’s slightly nicer than passing in a pre-made Sock because it allows testing the “create a socket of the right family” logic. (also ping @vasild)

Do you know what’s the minimum FreeBSD version it can be compiled on? Let’s bump the version bound to that.

Just tried on 14.0 with its default clang 16 and I get the same error. So we should either find a workaround or disable FreeBSD for this feature and a TODO comment. I don’t know how popular this is as a desktop distro?

Just tried on 14.0 with its default clang 16 and I get the same error. So we should either find a workaround or disable FreeBSD for this feature and a TODO comment. I don’t know how popular this is as a desktop distro?

i would feel bad disabling FreeBSD support after @vasild contributed the code for that, but if this gets close to merge and FreeBSD is still broken i’ll remove it (added a TODO). i expect #define typeof __typeof__ would go a long way to work around this error.

Finished the rest of my code review and tested 97ae5d4eefe4 again on Intel macOS 14.5, Ubuntu 24.04 and Windows.

I noticed that the renewal for IPv6 fails (at least on Ubuntu and macOS) with NO_RESOURCES. IPv4 renewal works fine. Ten minutes later it tries again and the mapping succeeds.

IIUC OPNsense uses miniupnpd 2.3.1, which is two years behind but at first glance I don’t see any recent fixes related to renewals, nor any open issues.s

One possible explanation could be that the router doesn’t implement the protocol correctly, in terms of when it allows renewal. If that’s the case, it may be worth implementing the recommended retry intervals. We should also log the approximate time remaining so it’s a bit easier to debug the router behavior.

Windows decided to quarantine bitocin-qt.exe (built with guix). I haven’t seen that before in earlier tests, so I wonder if it’s related to this change.

The timing suggests it happened 10 minutes after the ports were intially opened, so I’m guessing a renewal attempt triggered it, but I forgot to turn on more verbose logging.

I noticed that the renewal for IPv6 fails (at least on Ubuntu and macOS) with NO_RESOURCES. IPv4 renewal works fine. Ten minutes later it tries again and the mapping succeeds.

Is this after re-launching bitcoind? Mind that restarting will roll a new mapping nonce, which means that from the point of your router it’s a new application trying to get the port, which will prevent it from making the mapping until the old mapping expires. But that should go away after one cycle and not happen again the next one, as the nonce is stored and reused for renewals (as is specified in the RFC).

i doubt this has anything to do with specifiic intervals. If renewal is avoided while there is already a mapping at all then there will be holes in reachability, which is worse than some spurious errors.

Windows decided to quarantine bitocin-qt.exe (built with guix). I haven’t seen that before in earlier tests, so I wonder if it’s related to this change.

Possible, but that would be strange, given that we’re not communicating to any different ports than before. Were you building with libnatpmp before? (oh, a guix build, yes it will have libnatpmp enabled). Things like this make me so close to just closing this PR in frustration tbh.

Is this after re-launching bitcoind?

No, I hadn’t run the node in a while. At startup it opens the port, only renewal fails. Leaving the node running, this seems to happen half the time, as you would expect: port is opened, renewal fails, mapping expires, so opening succeeds again, renewal fails, etc.

Were you building with libnatpmp before?

No, I had that disabled in the past. I’ll see if I can trigger it again with more detailed logs. But you’re right, it might be an existing issue.

I downloaded v27.0 and tried it on Windows, but NAT-PMP doesn’t work at all there (natpmp: Port mapping failed). It tried turning the windows firewall off but that made no difference. It does on macOS ([mapport] natpmp: Port mapping successful. External address = ...).

No, I hadn’t run the node in a while. At startup it opens the port, only renewal fails. Leaving the node running, this seems to happen half the time, as you would expect: port is opened, renewal fails, mapping expires, so opening succeeds again, renewal fails, etc.

This is the opposite from what’s expected.

• But it only happens for IPv6, not IPv4. i wonder if that’s because IPv4 is the first mapping created? Maybe it doesn’t like the same nonce being used for multiple different mappings existing at the same time? Maybe try putting the IPv6 block first and see if it reverses the issue? Or at least makes it succeed for the first IPv6 address.

• Alternatively, maybe try generating a new nonce every call to PCPRequestPortMap. This violates the protocol but … if it’s implemented wrongly on the server this may trigger it to generate a fresh mapping. i dunno…

After a few minutes at renewal it complains with NO_RESOURCES.

Does it complain for all three mappings?

Yes, for all three.

I then recompiled and set the renewal interval at 6/8 - 7/8. Started the node again (using a fresh port). Same pattern: three mappings succeeed, at renewal all three complain about NO_RESOURCES. (And then a few minutes later they succesfully get a mapping again).

Actually this might be an existing bug: https://github.com/miniupnp/miniupnp/commit/7bd0877b8fd9a1c1c59cdf426b4640b3cee2bf61

I’ll see if I can convince OPNsense to ship >= 2.3.6, tracking at https://github.com/opnsense/plugins/issues/4003.

generating a new nonce every time for every mapping

That doesn’t seem like a good idea. IIUC the nonce is used to recognise the requesting application, and to distinguish a renew request from some other app trying to use the same port.

That doesn’t seem like a good idea. IIUC the nonce is used to recognise the requesting application, and to distinguish a renew request from some other app trying to use the same port.

It’s not supposed to be a good idea, but i wonder if it works around the issue with your router, if they implemented the protocol wrongly. i honestly have no idea what could be the problem here i’m just guessing.

I thought perhaps the issue was that miniupnpd checks whether at least half the lease time went by and would refuse if not. So then if the lease time was too far in the future, it wouldn’t allow renewal.

But looking through the source code it doesn’t appear to care about that. There’s a few places that can trigger a PCP_ERR_NO_RESOURCES reply, but so far those don’t offer an obvious explanation.

The pinhole code is mostly seperate from port mappings. Depending on pcp_msg_info->is_fw it will call CreatePCPMap_FW for an IPv6 pinhole. My guess is this function doesn’t recognise the renewal attempt as such, treats it as a new request and then fails. I need to figure out how to read the router log messages.

Figured out how to get verbose logging (https://github.com/opnsense/plugins/issues/4004).

It does log updating pinhole to before the failure, so it does find the existing route and knows that it should update. The line after that log statement calls upnp_update_inboundpinhole and interprets any failure as PCP_ERR_NO_RESOURCES. There’s only two ways that can fail:

1. If #if defined(USE_PF) || defined(USE_NETFILTER) is false, because it’ll return -42; /* not implemented */. But upnp_find_inboundpinhole has the same guard, so that can’t be it.

2. If update_pinhole fails, which can only fail if get_pinhole fails. That implies that the uid returned by find_pinhole is wrong.

I’m probably still missing something… I should test if deleting an IPv6 pinhole does work.

Deleting a pinhole succeeds. The log message is wrong (PCP: UDP port 8 mapping removed), which is an unrelated bug https://github.com/miniupnp/miniupnp/issues/743.

I also tried opening only a single pinhole, to rule out some race condition. But that doesn’t make a difference.

Anyway, I’m fairly sure it’s not a bug in this PR.