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

a2d67c320f8a28da98e6c8352bd67648a9b831a8: I think you need to keep this (and above) until -natpmp is removed.

Yes, good point.

nit:

    LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "pcp: Requesting port mapping for addr %s port %d from gateway %s\n", bind.ToStringAddr(), port, gateway.ToStringAddr());

73037f27fc21765414c298b171dfdeee130c549b: needs to be true, but you can actually drop this line.

Whoops, yes, it's setEnabled, not setting the default value 😅 .

73037f27fc21765414c298b171dfdeee130c549b: maybe add: "PCP is the successor to NAT-PMP.", in case someone who didn't read the release notes is confused why that option disappeared.

6cf4809c6b93e1720dfdfe4e3320cfd8939686b6: This is ::Warning worthy.

It's hidden as debug because at the moment this is shown when the user doesn't have IPv6. Could boost this to warning if we do the address check first, then don't bother looking for a default gateway if there's none. Will do that.

Will keep this open but leave it as-is for now. For IPv4 i'm not currently sure how to check if we have (Internet) networking besides checking for a default gateway, and i'd like to keep the two paths reasonably symmetric. Just checking for publicly routable addresses isn't going to cut it for IPv4.

The following patch uses a netlink socket to get the information from the kernel, that is supported on (at least) Linux and FreeBSD>=13.2:

<details> <summary>[patch] get default gateway using a netlink socket</summary>

--- a/src/test/netbase_tests.cpp
+++ b/src/test/netbase_tests.cpp
@@ -1,25 +1,34 @@
 // Copyright (c) 2012-2022 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <compat/compat.h>
 #include <net_permissions.h>
 #include <netaddress.h>
 #include <netbase.h>
 #include <netgroup.h>
 #include <protocol.h>
 #include <serialize.h>
 #include <streams.h>
 #include <test/util/setup_common.h>
 #include <util/strencodings.h>
+#include <util/syserror.h>
 #include <util/translation.h>
 
 #include <string>
 
 #include <boost/test/unit_test.hpp>
 
+#ifdef __linux__
+#include <linux/rtnetlink.h>
+#elif defined(__FreeBSD__)
+#include <netlink/netlink.h>
+#include <netlink/netlink_route.h>
+#endif
+
 using namespace std::literals;
 
 BOOST_FIXTURE_TEST_SUITE(netbase_tests, BasicTestingSetup)
 
 static CNetAddr ResolveIP(const std::string& ip)
 {
@@ -610,7 +619,100 @@ BOOST_AUTO_TEST_CASE(isbadport)
             ++total_bad_ports;
         }
     }
     BOOST_CHECK_EQUAL(total_bad_ports, 80);
 }
 
+std::optional<CNetAddr> QueryDefaultGateway(Network network)
+{
+    Assume(network == NET_IPV4 || network == NET_IPV6);
+
+    // Create a netlink socket.
+
+    const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
+    if (s < 0) {
+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", SysErrorString(errno));
+        return std::nullopt;
+    }
+    Sock sock{static_cast<SOCKET>(s)};
+
+    // Send request.
+
+    struct {
+        nlmsghdr hdr; ///< Request header.
+        rtmsg data; ///< Request data, a "route message".
+        nlattr dst_hdr; ///< One attribute, conveying the route destination address.
+        char dst_data[16]; ///< Route destination address. To query the default route we use 0.0.0.0/0 or [::]/0. For IPv4 the first 4 bytes are used.
+    } request;
+
+    // Whether to use the first 4 or 16 bytes from request.attr_dst_data.
+    const size_t dst_data_len = network == NET_IPV4 ? 4 : 16;
+
+    memset(&request, 0x0, sizeof(request));
+
+    request.hdr.nlmsg_type = RTM_GETROUTE;
+    request.hdr.nlmsg_flags = NLM_F_REQUEST;
+#ifdef __linux__
+    request.hdr.nlmsg_flags |= NLM_F_DUMP;
+#endif
+    request.hdr.nlmsg_len = NLMSG_LENGTH(sizeof(rtmsg) + sizeof(nlattr) + dst_data_len);
+    request.hdr.nlmsg_seq = 0; // Sequence number, used to match which reply is to which request. Irrelevant for us because we send just one request.
+    request.data.rtm_family = network == NET_IPV4 ? AF_INET : AF_INET6;
+    request.data.rtm_dst_len = 0; // Prefix length.
+#ifdef __FreeBSD__
+    request.data.rtm_flags = RTM_F_PREFIX;
+#endif
+    request.dst_hdr.nla_type = RTA_DST;
+    request.dst_hdr.nla_len = sizeof(nlattr) + dst_data_len;
+
+    if (sock.Send(&request, sizeof(request), 0) != sizeof(request)) {
+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "send() to netlink socket: %s\n", SysErrorString(errno));
+        return std::nullopt;
+    }
+
+    // Receive response.
+
+    char response[4096];
+    ssize_t response_len;
+    do {
+        response_len = sock.Recv(response, sizeof(response), 0);
+    } while (response_len < 0 && (errno == EINTR || errno == EAGAIN));
+    if (response_len < 0) {
+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "recv() from netlink socket: %s\n", SysErrorString(errno));
+        return std::nullopt;
+    }
+
+    for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {
+        rtmsg* r = (rtmsg*)NLMSG_DATA(hdr);
+        int remaining_len = RTM_PAYLOAD(hdr);
+        // Iterate over the attributes.
+        for (rtattr* attr = RTM_RTA(r); RTA_OK(attr, remaining_len); attr = RTA_NEXT(attr, remaining_len)) {
+            if (attr->rta_type == RTA_GATEWAY) {
+                if (network == NET_IPV4) {
+                    Assume(sizeof(in_addr) == RTA_PAYLOAD(attr));
+                    return CNetAddr{in_addr{.s_addr = *static_cast<decltype(in_addr::s_addr)*>(RTA_DATA(attr))}};
+                } else {
+                    Assume(sizeof(in6_addr) == RTA_PAYLOAD(attr));
+                    in6_addr gw;
+                    std::memcpy(&gw, RTA_DATA(attr), sizeof(gw));
+                    return CNetAddr{gw};
+                }
+            }
+        }
+    }
+
+    return std::nullopt;
+}
+
+BOOST_AUTO_TEST_CASE(netlink)
+{
+    for (const auto net : {NET_IPV4, NET_IPV6}) {
+        const auto gw{QueryDefaultGateway(net)};
+        if (gw.has_value()) {
+            printf("Default %s gateway: %s\n", GetNetworkName(net).c_str(), gw->ToStringAddr().c_str());
+        } else {
+            printf("No %s default gateway.\n", GetNetworkName(net).c_str());
+        }
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()

</details>

I find the netlink interface a somewhat difficult to grasp.

https://man7.org/linux/man-pages/man7/netlink.7.html https://man7.org/linux/man-pages/man7/rtnetlink.7.html https://man7.org/linux/man-pages/man3/rtnetlink.3.html https://stackoverflow.com/questions/11788326/extract-current-route-from-netlink-message-code-attached https://www.rfc-editor.org/rfc/rfc3549

Huh interesting. i didn't know netlink worked for multiple operating systems, that's much better.

Is it possible to get the scope_id for IPv6 addresses? At least my router gives me an link-scope address.

Edit: on linux this is RTA_OIF, it appears

Here is a standalone program to get the default gateway using a netlink socket:

<details> <summary>netlink_get_default_route.cc</summary>

#include <arpa/inet.h>
#include <assert.h>
#include <errno.h>
#include <net/if.h>
#include <netinet/in.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <unistd.h>

#ifdef __linux__
#include <linux/rtnetlink.h>
#elif defined(__FreeBSD__)
#include <netlink/netlink.h>
#include <netlink/netlink_route.h>
#endif

void QueryDefaultGateway(sa_family_t family)
{
    // Create a netlink socket.

    int sock = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
    if (sock < 0) {
        perror("socket(AF_NETLINK)");
        return;
    }

    // Send request.

    struct {
        nlmsghdr hdr; ///< Request header.
        rtmsg data; ///< Request data, a "route message".
        nlattr dst_hdr; ///< One attribute, conveying the route destination address.
        char dst_data[16]; ///< Route destination address. To query the default route we use 0.0.0.0/0 or [::]/0. For IPv4 the first 4 bytes are used.
    } request{};

    // Whether to use the first 4 or 16 bytes from request.attr_dst_data.
    const size_t dst_data_len = family == AF_INET ? 4 : 16;

    request.hdr.nlmsg_type = RTM_GETROUTE;
    request.hdr.nlmsg_flags = NLM_F_REQUEST;
#ifdef __linux__
    // XXX some strange behavior:
    // Linux IPv4 - this must be present, otherwise no gateway is found
    // Linux IPv6 - this must be present, otherwise no gateway is found
    // FreeBSD IPv4 - does not matter, the gateway is found with or without this
    // FreeBSD IPv6 - this must be absent, otherwise no gateway is found
    request.hdr.nlmsg_flags |= NLM_F_DUMP;
#endif
    request.hdr.nlmsg_len = NLMSG_LENGTH(sizeof(rtmsg) + sizeof(nlattr) + dst_data_len);
    request.hdr.nlmsg_seq = 0; // Sequence number, used to match which reply is to which request. Irrelevant for us because we send just one request.
    request.data.rtm_family = family;
    request.data.rtm_dst_len = 0; // Prefix length.
    //request.data.rtm_table = RT_TABLE_MAIN;
    //request.data.rtm_protocol = RTPROT_STATIC;
    //request.data.rtm_scope = RT_SCOPE_UNIVERSE;
    //request.data.rtm_type = RTN_UNICAST;
#ifdef __FreeBSD__
    // XXX some strange behavior:
    // Linux IPv4 - this must be absent, otherwise no gateway is found
    // Linux IPv6 - this must be absent, otherwise no gateway is found
    // FreeBSD IPv4 - does not matter, the gateway is found with or without this
    // FreeBSD IPv6 - this must be present, otherwise no gateway is found
    request.data.rtm_flags = RTM_F_PREFIX;
#endif
    request.dst_hdr.nla_type = RTA_DST;
    request.dst_hdr.nla_len = sizeof(nlattr) + dst_data_len;

    if (send(sock, &request, request.hdr.nlmsg_len, 0) != request.hdr.nlmsg_len) {
        fprintf(stderr, "send() failed to send %u bytes\n", request.hdr.nlmsg_len);
        close(sock);
        return;
    }

    // Receive response.

    char response[4096];
    ssize_t response_len;
    do {
        response_len = recv(sock, response, sizeof(response), 0);
    } while (response_len < 0 && (errno == EINTR || errno == EAGAIN));
    if (response_len < 0) {
        fprintf(stderr, "recv(): %s\n", strerror(errno));
        close(sock);
        return;
    }

    for (nlmsghdr *hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {
        rtmsg* r = (rtmsg*)NLMSG_DATA(hdr);
        int remaining_len = RTM_PAYLOAD(hdr);

        // Iterate over the attributes.
        rtattr* rta_gateway = nullptr;
        int scope_id = 0;
        for (rtattr *attr = RTM_RTA(r); RTA_OK(attr, remaining_len); attr = RTA_NEXT(attr, remaining_len)) {
            if (attr->rta_type == RTA_GATEWAY) {
                rta_gateway = attr;
            } else if (attr->rta_type == RTA_OIF) {
                assert(sizeof(int) == RTA_PAYLOAD(attr));
                memcpy(&scope_id, RTA_DATA(attr), sizeof(scope_id));
            }
        }

        // Found gateway?
        if (rta_gateway != nullptr) {
            char buf[256];
            printf("%s gateway: %s",
                   family == AF_INET ? "IPv4" : "IPv6",
                   inet_ntop(r->rtm_family, RTA_DATA(rta_gateway), buf, sizeof(buf)));
            if (family == AF_INET6) {
                printf(", scope id: %d\n", scope_id);
            } else {
                printf("\n");
            }
        }
    }

    close(sock);
}

int main(int argc, char** argv)
{
    QueryDefaultGateway(AF_INET);
    QueryDefaultGateway(AF_INET6);
    return 0;
}

</details>

Ideally the #ifdef __linux__ / #ifdef __FreeBSD__ parts should not be needed. I am not sure if this is due to the above program doing something wrong or is due to a difference in Linux vs FreeBSD implementations. @AlexanderChernikov, @markjdb, any ideas?

I realized that in the IPv4 case this would send the trailing 16-4=12 bytes from request.dst_data[]. This seems harmless, but better send exactly what's needed:

    if (sock.Send(&request, request.hdr.nlmsg_len, 0) != request.hdr.nlmsg_len) {
        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "send() to netlink socket: %s\n", SysErrorString(errno));

Also, I am not sure if we should worry about partial writes with netlink sockets, maybe the send can be interrupted? There is a convenience method Sock::SendComplete():

    try {
        CThreadInterrupt intr;
        sock.SendComplete(Span{reinterpret_cast<const unsigned char*>(&request), request.hdr.nlmsg_len}, 5s, intr);
    } catch (const std::runtime_error& e) {
        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "writing to netlink socket: %s\n", e.what());
        return std::nullopt;
    }

It's a datagram (packet socket), so partial writes and reads cannot happen. Every send is interpreted as a new packet. Using SendComplete would be a bug.

Truncated and corrupted packets could happen in the case of UDP, but as NETLINK is a communication interface with the kernel, that would be rare. No retries are needed. Would still want to detect it and error out gracefully, though, for robustness.

I realized that in the IPv4 case this would send the trailing 16-4=12 bytes from request.dst_data[]. This seems harmless, but better send exactly what's needed

Agreed, will change that.

bebfafcf98d8ed1432407b7603991d54f7cc26c2: it would be useful to have a quick recap here of which strategy is used by QueryDefaultGateway for which OS.

I think it will be (slightly) more readable to have a single QueryDefaultGateway implementation which then calls QueryDefaultGatewayWindows, QueryDefaultGatewayMac and QueryDefaultGatewayLinuxBSD.

It can start with:

Assume(network == NET_IPV4 || network == NET_IPV6)

and end with

#else return std::nullopt;

Agree, i think i would prefer QueryDefaultGatewayNetlink QueryDefaultGatewaySysctl QueryDefaultGatewayWin32 -- only the WIN32 one is truly OS specific, the other ones are POSIX-ish

OK, i went for QueryDefaultGatewayImpl for now, with an outer function. Naming it different things on different platforms means having to repeat the #ifdef forest, which isn't really worth the slight increase in clarity imo.

bebfafcf98d8ed1432407b7603991d54f7cc26c2 presumably netlink only exists in (the rather recent) FreeBSD >= 13.2? For older versions we need to use the same method as macOS? If so, then we probably need to check this stuff in configure.

That said, it looks like FreeBSD 13.2 is the oldest supported release and 12 is pretty much unusable: https://forums.freebsd.org/threads/freebsd-12-2-stable-pkg-update-failed.92034/#post-640223

I was not able to build bitcoind on a FreeBSD 13.2 VM:

<img width="723" alt="bsd 13" src="https://github.com/bitcoin/bitcoin/assets/10217/8b4d0ee5-e8e7-4318-8c22-67791a77974c">

(I don't have Virtual Box guest editions installed, so having a hard time copying the output)

If so i would prefer not supporting it at all for older FreeBSD. It's so rare compared to the other operating systems already, and most of the userbase will be setting their own firewall. But we indeed need to check that it doesn't cause a compilation error.

If so i would prefer not supporting it at all for older FreeBSD.

Seems fine to me.

Added a version check for at least 13.2.

13.2 is from April 2023: https://www.freebsd.org/releases/13.2R/announce/ On FreeBSD it is easy and smooth to upgrade the OS (at least my experience since FreeBSD 4.x) even across major versions (e.g. 13 -> 14). Seems fine to only support >= 13.2.

Right, that's good to know. However the current problem is that we can't compile this for any FreeBSD version. Looks like the MLMSG etc macros don't work in C++, because typeof is used.

@laanwj One possibility is moving the code to a separate .c file, perhaps?

That would probably work, but that'd be a really invasive workaround build system wise.

Yes, please don't do that :)

My expectation is that a #define typeof __typeof__ around just the use of that macro on FreeBSD could fix it. Just haven't been able to try it out (and still hope someone who has a clue about FreeBSD has some kind of obvious solution).

This is the upstream fix: https://github.com/freebsd/freebsd-src/pull/1070.

The suggested workaround to define typeof is ok:

diff --git i/src/util/netif.cpp w/src/util/netif.cpp
index 845b8aed1d..1840974a83 100644
--- i/src/util/netif.cpp
+++ w/src/util/netif.cpp
@@ -9,18 +9,24 @@
 #include <logging.h>
 #include <netbase.h>
 #include <util/check.h>
 #include <util/sock.h>
 #include <util/syserror.h>
 
+#ifdef __FreeBSD__
+#include <osreldate.h>
+#endif
+
 // Linux and FreeBSD 13.2+
-#if defined(__linux__) || (defined(__FreeBSD__) && __FreeBSD_version >= 1302000)
+#if defined(__linux__) || __FreeBSD_version >= 1302000
 
 #if defined(__linux__)
 #include <linux/rtnetlink.h>
 #elif defined(__FreeBSD__)
+// Workaround https://github.com/freebsd/freebsd-src/pull/1070.
+#define typeof __typeof
 #include <netlink/netlink.h>
 #include <netlink/netlink_route.h>
 #endif
 
 static std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
 {
@@ -77,14 +83,13 @@ static std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
     } while (recv_result < 0 && (errno == EINTR || errno == EAGAIN));
     if (recv_result < 0) {
         LogPrintLevel(BCLog::NET, BCLog::Level::Error, "recv() from netlink socket: %s\n", NetworkErrorString(errno));
         return std::nullopt;
     }
 
-    size_t response_len = static_cast<size_t>(recv_result);
-    for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {
+    for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) {
         rtmsg* r = (rtmsg*)NLMSG_DATA(hdr);
         int remaining_len = RTM_PAYLOAD(hdr);
 
         // Iterate over the attributes.
         rtattr *rta_gateway = nullptr;
         int scope_id = 0;

The above patch does two more things:

• __FreeBSD_version was not defined at the time it was checked, so __FreeBSD_version >= 1302000 was always false. Include the header that defines it.
• Silence a warning about signed vs unsigned integer comparison.

I tested this by reverting the upstream fix - s/__typeof/typeof in my /usr/include/netlink/netlink.h. Confirming that it gives the typeof error, then applying to above workaround and confirming that it compiles.

Thanks!

Silence a warning about signed vs unsigned integer comparison.

Oh no. Pretty sure i had to do the response_len thing to work around another error about signed-unsigned integer comparison in the i686 CI run.

Edit: pushed this change as-is, let's see.

On my Linux distro I have:

struct nlmsghdr {          
        __u32           nlmsg_len;
...
#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
                           (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
                           (nlh)->nlmsg_len <= (len))

So, the second argument of NLMSG_OK() - len is compared against 1. (int)sizeof... and 2. ...->nlmsg_len (which is __u32). The only difference on FreeBSD is that in 2. it is compared against (int)(_hdr)->nlmsg_len - an explicitly cast nlmsg_len to int (nlmsg_len is uint32_t on FreeBSD).

I do not get a warning when compiling that code on Linux:

    ssize_t response_len;
...
    for (nlmsghdr *hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {

I am not sure why. I should get a warning about __u32 <= ssize_t, right?

I am not sure why. I should get a warning about __u32 <= ssize_t, right?

Comparing an unsigned type against a larger signed type does what one would expect: it gets casted to the larger signed type before comparison. This is why on 64-bit platforms this is fine. However on 32-bit platforms, ssize_t is 32 bit. So there it warns.

I see, then defining recv_result as int64_t instead of ssize_t should be ok on all platforms?

Will try. Introducing a platform depenendent typedef just for this would be a bit silly.

Concept ACK on keep just using -natpmp for both PCP and NAT-PMP.

(you lost the -)

Whoops, fixed

82b0bff51fedff2ce5e47cae1c75b9172766d8ef: the SysErrorString documentation says you should call NetworkErrorString, though that will in turn just call SysErrorString, since this is never called under WIN32.

Right, will update this (though yeah for POSIX operating systems there's no difference).

82b0bff51fedff2ce5e47cae1c75b9172766d8ef Why only on linux? It seems to exist on FreeBSD too: https://man.freebsd.org/cgi/man.cgi?netlink(4)

For the FreeBSD versus Linux differences see the comments in the standalone tool here: #30043 (review) The netlink calls behave differently on Linux and FreeBSD, we don't know why this is.

Ah, some of these comments are worth preserving until we know more or can point to clear documentation elsewhere. I compressed them a bit:

    // Linux IPv4 / IPv6 - this must be present, otherwise no gateway is found
    // FreeBSD IPv4 - does not matter, the gateway is found with or without this
    // FreeBSD IPv6 - this must be absent, otherwise no gateway is found
    request.hdr.nlmsg_flags |= NLM_F_DUMP;

#ifdef __FreeBSD__
    // Linux IPv4 / IPv6 this must be absent, otherwise no gateway is found
    // FreeBSD IPv4 - does not matter, the gateway is found with or without this
    // FreeBSD IPv6 - this must be present, otherwise no gateway is found
    request.data.rtm_flags = RTM_F_PREFIX;

Sure, will add that. FWIW, this is why i initially went with parsing the route tables from /proc/net/..., for Linux that's the most straightforward implementation. Netlink is a bit finnicky, though it should be stable (for the same OS) because it's what the tools like ip use.

82b0bff51fedff2ce5e47cae1c75b9172766d8ef: IIUC this gets the scope id (https://datatracker.ietf.org/doc/html/rfc4007), but we don't do anything with that except in IPv6ToString. So maybe we should just ignore this value (CNetAddr initialiser defaults it to 0).

If we can't drop it, can we be sure that we encounter RTA_OIF before RTA_GATEWAY? Otherwise if (rta_gateway != nullptr) could trigger prematurely.

I see this was added here: #30043 (review), and I see it's not actually ignored, e.g. GetSockAddr uses it. ~But when we connect to the default gateway, only m_addr is copied by GetSockAddr (which is called by PCPRequestPortMap).~ (see below)

The scope ID is extremely important for the default gateway, as it tends to be a scope-local address (it wouldn't work at all here without that).

If we can't drop it, can we be sure that we encounter RTA_OIF before RTA_GATEWAY? Otherwise if (rta_gateway != nullptr) could trigger prematurely.

AFAIK the order of the attributes within a record can be arbitrary. But how can this go wrong? The rta_gateway check is only after going over all the attributes, right?

GetSockAddr does copy the scope_id too, see https://github.com/bitcoin/bitcoin/blob/master/src/netaddress.cpp#L883

The rta_gateway check is only after going over all the attributes, right?

Oh wait, I misread the indentation, yes.

GetSockAddr does copy the scope_id too,

Indeed it does

The scope ID is extremely important for the default gateway, as it tends to be a scope-local address.

Aha: https://blogs.infoblox.com/ipv6-coe/fe80-1-is-a-perfectly-valid-ipv6-default-gateway-address/

Unlike the linux code, here it does matter to call NetworkErrorString, because it calls Win32ErrorString, which calls FormatMessage(W) as the docs recommend: https://learn.microsoft.com/en-us/windows/win32/api/iphlpapi/nf-iphlpapi-getbestinterfaceex

My understanding is that using strerror_s (called by SysErrorString) here would be wrong: https://stackoverflow.com/a/20057368

I wonder if we can prevent doing that by accident.

That said, this Windows code is a lot simpler than the Linux stuff above (ducks...).

OHH, my thinking was that it's a WIN32 API function, not a network function. So i thought SysErrorString would be correct. But i think you're right. "SysError" is more like "posix errno emulation error". Which is not what is needed here.

That said, this Windows code is a lot simpler than the Linux stuff above (ducks...).

You mean that centrally-controlled proprietary OSes sometimes have well-documented API's that consider use-cases, while FOSS often uses haphazard grabbag API's that were grown in accordance with one tool (where everyone is expected to parse text output of-)... or some custom library, with equally confusing interface. you can just say that out loud here you know... 😓 Standards like RFCs, and POSIX (ignoring everyone's custom extensions) are rare exceptions.

Maybe also check that NextHop is not 0.0.0.0 (::/0 for IPv6 below). It's not entirely clear to me from the documentation if that can realistically happen, but doesn't hurt to check either.

Maybe a general "is the default gateway address sane" check might sense? though, up to some point if the OS returns a weird address who are we to question it. Can't get too paranoid about that. The worst that could happen is sending to 0.0.0.0, which wouldn't result in any bad things beyond an error.

Or do you mean this from a "what happens if there is no default gateway configured" angle? yes, we'll have to check what it does in that case. Though i'd expect the call to GetBestRoute to fail.

OK, you're right, this could happen: https://learn.microsoft.com/en-us/windows/win32/api/netioapi/ns-netioapi-mib_ipforward_row2

NextHop

Type: SOCKADDR_INET

For a remote route, the IP address of the next system or gateway en route. If the route is to a local loopback address or an IP address on the local link, the next hop is unspecified (all zeros). For a local loopback route, this member should be an IPv4 address of 0.0.0.0 for an IPv4 route entry or an IPv6 address of 0::0 for an IPv6 route entry.

Maybe this is true for the other operating systems as well, if the gateway address is .IsBindAny() it should be considered as absent.

Edit: done

Alternatively you could store net.route.0.inet[4].flags.gateway here and use sysctlbyname below, avoiding the need to construct mib[]. Not sure if that's better though, because it's nice to be able to lookup constants like NET_RT_FLAGS in headers.

i prefer using constants to string based APIs, if given the choice. This avoids say, typos.

Seems we could factor out AddressFamilyFromNetwork at least, this is repeated in literally every implementation 😄

Edit: in netaddress.h there is CService::GetSAFamily() -- so close but not... sigh. Edit: done

Could make this slightly more readable with comments and two nullptr:

if (sysctl(/*name=*/mib, /*namelen=*/sizeof(mib) / sizeof(int), /*oldp=*/nullptr, /*oldlenp=*/&l, /*newp=*/nullptr, /*newlen=*/0) < 0) {

i'm not sure it makes sense to duplicate documentation that's already in the manual pages. Like if we're doing this here, why not for every system call we make. Do parameter names like *newp= oldp= even elucidate much?

Agree re using nullptr where possible.

oldlenp= does because it's key to the trick of how we get the length first. I find these variable name hints useful to e.g. search for them.

if (sysctl(/*name=*/mib, /*namelen=*/sizeof(mib) / sizeof(int), /*oldp=*/buf.data(), /*oldlenp=*/&l, /*newp=*/nullptr, /*newlen=*/0) < 0) {

A useful hint to the reader what's going on here, and where it's documented:

    // The size of the available data can be determined by calling sysctl() with
    // the NULL argument for oldp. See sysctl(3).

I guess you can't do something closer to for (const struct rt_msghdr* rt : buf) (somehow passing in rt->rtm_msglen).

i don't think so, the pointer needs to advance by the size of the specific message, which is part of that message.

Possible alternative, makes it a bit more clear that we rely on rt->rtm_msglen to be correct. A for loop gives a false sense of safety. No strong feelings though.

    std::byte* p = buf.data();
    while (true) {
        rt = (const struct rt_msghdr*)p;
        // ...

        p += rt->rtm_msglen;
        if (p == buf.data() + buf.size()) break;
        assert(p < buf.data() + buf.size());
    }

Do we want to check rt->rtm_errno first?

(if there's an error then probably no bit flags are set, but who knows)

Well, i guess it wouldn't hurt checking (or adding an Assume), but i don't think the routing table is supposed to contain error entries (also these would generally have rtm_addrs==0).

Mmm, I thought perhaps it would return a single error entry to indicate failure.

From what i understand, the error signalling and many of the flags and operations in the rt_msghdr are used with PF_ROUTE sockets, interactively. sysctl stores a read-only copy of the routing table data so that non-root users can access it. Granted, it's kind of a weird API.

// We only read from this address if a rtm_addrs bit flag is set.

Yes, it's a minor optimization, we could always construct a CNetAddr if we wanted, but if we know we're not going to use it anyway might as well skip it.

Edit: oh, i get what you mean. Might be better to just keep a byte pointer and only cast to an struct sockaddr * when we need it? all this back and forth casting doesn't make things clearer.

I'm not sure, but the reason I wrote this comment is because initially I thought: yikes, what if this is out of bound?

How about:

// rt_msghdr is followed by zero or more sockaddrs, as indicated by rtm_addrs
auto sa = (const struct sockaddr*)(p + sizeof(rt_msghdr));

i moved the cast to the if() inside the inner loop.

// Skip sockaddr entries for bit flags we're not interested in,
// move cursor.

This seems quite brittle and I don't fully understand it. I did test that it seems necessary, e.g. doing sa++; causes it to not map IPv6 ports. If I look at struct sockaddr_storage I'm seeing _SS_ALIGNSIZE (sizeof(__int64_t), but using uint64_t instead of uint32_t in your ROUNDUP also doesn't work.

i'm not sure either, i see it in some other libraries, but no alignment is mentioned in the manual page for route at all (which implies they're just back-to-back, no matter what). Leaving out ROUNDUP works fine btw. Might just delete it.

Edit: yes, using long or uint64_t breaks IPv6. Also they're variable-length so doing just sa++ won't work.

Also they're variable-length

That makes sense. Whereas rt_msghdr is fixed length I guess, so you're able to use rt + 1 above (I'm still a bit terified that compilers don't care).

Right, let's add sizeof(rt_msghdr) manually instead of doing the +1 trick.

That makes sense. Whereas rt_msghdr is fixed length I guess, so you're able to use rt + 1 above (I'm still a bit terified that compilers don't care).

If we can hardcode the assumption that sa_len is the first byte (which it is) then we can avoid casting and dereferencing sockaddr, anywhere. E.g. when we know the length we can memcpy it into a sockaddr_storage first.

Currently i have the following, having changed the pointer arithmetic to byte offsets:

    // Iterate over messages (each message is a routing table entry).
    for (size_t msgptr = 0; msgptr < buf.size(); ) {
        Assume((msgptr + sizeof(rt_msghdr)) <= buf.size());
        const struct rt_msghdr* rt = (const struct rt_msghdr*)(buf.data() + msgptr);
        // Iterate over addresses within entry, get destination and gateway (if present).
        // Pointer to address data within message, starts after header.
        size_t saptr = msgptr + sizeof(rt_msghdr);
        size_t next_msgptr = msgptr + rt->rtm_msglen;
        Assume(next_msgptr <= buf.size());
        std::optional<CNetAddr> dst;
        std::optional<CNetAddr> gateway;
        for (int i = 0; i < RTAX_MAX; i++) {
            if (rt->rtm_addrs & (1 << i)) {
                Assume((saptr + 1) <= next_msgptr);
                const struct sockaddr* sa = (const struct sockaddr*)(buf.data() + saptr);
                Assume((saptr + sa->sa_len) <= next_msgptr);
                if (i == RTAX_DST) {
                    dst = FromSockAddr(sa);
                } else if (i == RTAX_GATEWAY) {
                    gateway = FromSockAddr(sa);
                }
                // Skip to next address.
                saptr += sa->sa_len;
            }
        }
        // Found default gateway?
        if (dst && gateway && dst->IsBindAny()) { // Route to 0.0.0.0 or :: ?
            return *gateway;
        }
        // Skip to next message.
        msgptr = next_msgptr;
    }

Edit: add anti-overflow assumptions.

That indeed looks better.

The *ptr variables are no longer actually pointers, so I suggest renaming them:

msgptr -> msg_pos within entry - within message saptr -> sa_pos next_msgptr -> next_msg_pos

Let's set and check next_msgptr before saptr and make it const.

Can you add "to gateway x.x.x.x" here? I'm getting this error on your latest commit, three times, but I can't tell if it's IPv4, IPv6 or both.

Strangely I do get four successful "Added mapping pcp" messages (1 on IPv4, 3 on IPv6).

Update: this happens when I'm connected with a physical LAN cable and wifi. So the warning was safe to ignore, but I'm still curious about it.

If it appears after the IPv4 gateway log message it's for the IPv4 gateway, if it's after the IPv6 one it's for the IPv6 gateway. No more than two gateways are ever used. i don't think adding it to every log message is worth it.

Update: this happens when I'm connected with a physical LAN cable and wifi. So the warning was safe to ignore, but I'm still curious about it.

Right i think that's the reason-if you have multiple internet connections, then mapping the IPv6 addresses connected to the other one (that's not the default gateway) will fail. This is a scenario too complex for automatic mapping to handle, anyhow. Glad to hear some addresses were still mapped correctly.

(fairly sure this issue does not arise with IPv4, because it maps one port on the internal address toward the default gateway)

I'm a bit surprised that when we ask for the default gateway, we get results for both network connections. But I guess we're not asking for the default gateway on macOS, but rather all of them?

Oh I think I get it. In ProcessPCP (mapport.cpp) we call PCPRequestPortMap for every IPv6 address we have, as determined by GetLocalAddresses().

Each port map request is made to the default gateway. This is (usually) a scope-local addresses, see #30043 (review) So unlike with IPv4, where a gateway like 192.168.1.1 is reachable through both network connections, the IPv6 default gateway is only reachable through one connection.

In PCPRequestPortMap we create a socket and bind it to the intended destination address. I'm still a bit puzzled why sock.Connect doesn't fail and instead sock.Send is where the error is triggered. I suppose that's because that's where the OS realizes there's no route and gives up? That's indeed what the error No route to host (65) implies.

And indeed we don't have to "fix" this. Section 2.3 says:

for a given IP address of a host, only one default route exists to reach other hosts on the Internet from that source IP address

PCP itself doesn't support this kind of setup either.

The only thing that matters is that nodes in the outside world can connect to us, we don't care which of the two network connections ends up getting used.

I guess if I were to turn off the wifi, and that happened to have been the default gateway, I won't get new inbound connections until 20 minutes have passed and we request a new mapping via the LAN default gateway. Haven't tested this.

typo: portmap

// We only attempt renewal once.

The spec recommend trying again with ever shorter intervals, but no less than 4 seconds, as the deadline approached. But it seems fine to not bother.

If renewal fails we fall out of the loop, ProcessPCP returns. At that point we fall back to UPNP (if enabled) and/or try again 5 minutes later.

Renewal for NAT-PMP works the same it seems, see towards the end of https://datatracker.ietf.org/doc/html/rfc6886#section-3.3

The spec recommend trying again with ever shorter intervals, but no less than 4 seconds, as the deadline approached. But it seems fine to not bother.

Right, the retrying is not according to spec-because i had the same thought. It seems overkill to adaptive timing for something that's meant to interact with the local router. If it doesn't have enough capacity to handle a PCP/NAT-PMP packet it sure won't be routing a lot of traffic. i was more afraid that bitcoind (or the system it's running on) itself is blocked (due to heavy verification i/o load) that i implemented the "start about halfway with renewal" instead of having a margin of a minute :smile:

If renewal fails we fall out of the loop, ProcessPCP returns. At that point we fall back to UPNP (if enabled) and/or try again 5 minutes later.

Indeed, that's my intent. Network configuration can always change, either by the user logging into another network or installing a different router/firmware update, so i think it's a feature that it keeps trying and it's otherwise stateless.

It seems like the gateway can keep us waiting forever by sending an invalid response at least once per second. Should we give up at some point?

yes, the proper way would be to set a deadline with a monotonic clock, instead of using the same timeout every time though, everything considered, the gateway can only hold up this thread; it's not like the rest of bitcoind is waiting on it, and if the gateway is not to be trusted then it's not like we're going to get any useful mappings anyway

The chrono timepoint/duration stuff kind of confuses me. Would this be the idea? (assuming timeout is per retry). sock.Wait takes milliseconds while the steady clock returns microseconds, so the cast seems unavoidable.

         }
 
         // Wait for response(s) until we get a valid response, a network error, or time out.
-        while (true) {
+        auto cur_time = std::chrono::time_point_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now());
+        auto deadline = cur_time + timeout;
+        while ((cur_time = std::chrono::time_point_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now())) < deadline) {
             Sock::Event occurred = 0;
-            if (!sock.Wait(std::chrono::milliseconds(1000), Sock::RECV, &occurred)) {
+            if (!sock.Wait(deadline - cur_time, Sock::RECV, &occurred)) {
                 LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s: Could not wait on socket: %s\n", protocol, NetworkErrorString(WSAGetLastError()));
                 return std::nullopt; // Network-level error, probably no use retrying.
             }

I guess this could work if timeout isn't too long. Since if the router doens't support NAT-PMP / PCP it's not going to reply, it delays when we fall back to UPNP. But a few seconds seems fine.

The timeout is still one second per try (so three seconds in total maximum, given current retries), it's just not possible to extend it indefinitely anymore by sending rejected packets.

bfde83050ece1f617efdb4098b5efeeb1a08b65e nit: UPnP, PCP or NAT-PMP

Sure, ok, really i'm just considering NAT-PMP to be PCPv0.

RPC -> RFC (in a few other places too)

Continuing the discussion from #30043 (comment), so that messages are grouped together, not scattered in the main PR thread.

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)? ...

Almost. Right now we have the "pointer"

std::function<std::unique_ptr<Sock>(const sa_family_t&)> CreateSock;

which in the real code points to the function

std::unique_ptr<Sock> CreateSockOS(sa_family_t address_family);

Tests that wish to mock the sockets redirect the pointer to another one CreateSockWhateverMockedStuff().

That address_family is the first argument to the socket(2) syscall. What's needed is to extend CreateSock*() with the other two arguments - type and protocol. I can do that. @Sjors, are you ACK on the first 2 commits of #26812? If yes, should I create a separate PR with those 2 commits?

@vasild I haven't had a chance to thoroughly review the second commit. Making a separate PR could help.

SGTM.

That address_family is the first argument to the socket(2) syscall. What's needed is to extend CreateSock*() with the other two arguments - type and protocol. I can do that.

Yes, for specifying UDP (and NETLINK, but i do not intend to make a test framework for that) it would need all three arguments.

Extended CreateSock() in the topmost commit in https://github.com/vasild/bitcoin/commits/extend_CreateSock/. Separate PR or include in this PR?

This PR is already quite large, so let's make a fresh one that this can rebase on if needed.

Agree adding fuzzing/testing makes sense as a follow-up PR. It's already big enough, and also i don't want to interfere with review by doing more active development here than address review comments.

Opened #30202 "netbase: extend CreateSock() to support creating arbitrary sockets"

Making a separate PR could help

Done, the first two commits from #26812 extracted into a separate PR: https://github.com/bitcoin/bitcoin/pull/30205

I got a bit confused about these two new PRs, but I guess they are orthogonal.

a8818568a44e02c0346ef37cde9155191f8b3df1: __FreeBSD_version can be lowered to 1302000 as per the comment (or 1302001 which I tested on). Although you need kldload /boot/kernel/netlink.ko.

sure, we could do that again, but i don't want people to have to bother with weird error messages and having to load kernel modules, probably this should work out of the box or not at all

Good point, maybe just add a comment about this. FreeBSD 13 won't be EOL for another two years, so someone else might wonder.

The comment says 13.2+ but the code says 1400000

3fdc97a2bf7d1ec774d2ffa00502188158c64724: this line could be moved to the miniupnpc section above:

Note: UPnP support will be compiled in, but disabled by default.

3fdc97a2bf7d1ec774d2ffa00502188158c64724: I don't know why this was here in the first place, seems fine to drop.

57e23f3b4d9cc5bdba421e47def9f3b2335d4cfb: nit, no need to drop this

In commit "net: Add PCP and NATPMP implementation"

Put a namespace { ... } // namespace around all of the functions/definitions/constants that are not in the .h file, and drop the static (just a nit, but anonymous namespaces are considered more modern).

In commit "net: Add PCP and NATPMP implementation":

Nit: check_packet({response, recvsz}) should work too (if no type is listed, the corresponding constructor of the declared type is used).

Unfortunately, that gives a new warning, because the int isn't the exact type:

../../src/util/pcp.cpp:258:41: warning: narrowing conversion of ‘recvsz’ from ‘int’ to ‘std::size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
  258 |             if (check_packet({response, recvsz})) {

Could add a static_cast but i don't think it's much of an improvement in that case?

In that case you could use check_packet(Span(response, recvsz)) (the <uint8_t> can be automatically deducted).

In commit "net: Add PCP and NATPMP implementation"

The const before Span<const uint8_t> has no effect (it's an argument that's passed by value). You can drop it while still keeping the one in the actual lambdas passed to this function (if you want to prevent the body of those lambdas from modifying the copy of the Span they receive).

In commit "net: Add PCP and NATPMP implementation"

Is this internal address expected to be different than the gateway variable that's passed in? It seems this internal address is only used to convert to a CService at return time from this function, so if it's equal to gateway, this seems like a very roundabout way of getting there.

The internal address is our internal address toward the gateway, not the gateway's address (so the address that the port is to be forwarded to from the external address). For IPv6 this is the same as the public address, and we explicitly bind to it before communicating (getsockname will just get it back, you're right). For IPv4 it could be anything in the router's private subnet, so we bind to INADDR_ANY and need to get it with getsockname.

In commit "net: Add PCP and NATPMP implementation"

Nit, if you want somewhat more brevity:

using namespace std::chrono_literals;

... NATPMPRequestPortMap(..., std::chrono::milliseconds timeout_per_try = 1s);

(and below)

It would be great to cut down on the verbosity here, but as this is a header, i don't think we want to use using there because that will leak into anything that includes it? Or am i misunderstanding?

@laanwj It's true that you generally shouldn't use using namespace std; in headers, but the C++ Core Guidelines actually have an exception for the standard literals. The reason being that literals without underscore are reserved for the standard library, so there is no risk of namespace pollution.

In commit "net: Add PCP and NATPMP implementation"

Nit: space after type

Now that #30202 has been merged this can be:

     // Create a netlink socket.
-    const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
-    if (s < 0) {
+    auto sock{CreateSock(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
+    if (!sock) {
         LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", NetworkErrorString(errno));
         return std::nullopt;
     }
-    Sock sock{static_cast<SOCKET>(s)};

     // Send request.

and replace sock.Send() with sock->Send() below (same for Recv()).

This would allow mocking of the socket creation.

sure, though i don't intend to add mocking for netlink sockets (that's just too OS specific), but for the UDP socket creation it'd be useful

    // Whether to use the first 4 or 16 bytes from request.dst_data.

If it happens that sizeof(in_addr) is not equal to RTA_PAYLOAD(rta_gateway) then I think it is better to not allow this to continue to the memcpy() because it could read past the end of the buffer. If we don't want to stop a release/production program for this with an assert(), then log + return std::nullopt; is maybe better than copying random undefined bytes around and interpreting them as an IP address.

(same for some Assume()s in the Apple implementation)

It's an OS API, it's very unlikely to fail (or at least, in a way that returns corrupted data), so i don't want to add a specific log message for it. i'm fine with it just returning nullptr. Will just merge these checks into the if() above them.

nit, can drop the parameter name for an unused parameter and the void cast:

static std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t)
{
    return std::nullopt;

style: { should be on the same line as if or for (also in a few places below).

style: make more doxygen friendly:

 //! Try to open a port using RFC 6886 NAT-PMP. IPv4 only.
 //!
-//! * gateway: Destination address for PCP requests (usually the default gateway).
-//! * port: Internal port, and desired external port.
-//! * lifetime: Requested lifetime in seconds for mapping. The server may assign as shorter or longer lifetime. A lifetime of 0 deletes the mapping.
-//! * num_tries: Number of tries in case of no response.
+//! [@param](/bitcoin-bitcoin/contributor/param/)[in] gateway Destination address for PCP requests (usually the default gateway).
+//! [@param](/bitcoin-bitcoin/contributor/param/)[in] port Internal port, and desired external port.
+//! [@param](/bitcoin-bitcoin/contributor/param/)[in] lifetime Requested lifetime in seconds for mapping. The server may assign as shorter or longer lifetime. A lifetime of 0 deletes the mapping.
+//! [@param](/bitcoin-bitcoin/contributor/param/)[in] num_tries Number of tries in case of no response.
 //!
-//! Returns the external_ip:external_port of the mapping if successful, otherwise a MappingError.
+//! [@return](/bitcoin-bitcoin/contributor/return/) The external_ip:external_port of the mapping if successful, otherwise a MappingError.

before:

after:

"RFC6887 section 1.1" should be "RFC6886 section 1.1"?

https://www.rfc-editor.org/rfc/rfc6886#section-1.1

nit: maybe worth logging also num_tries, e.g. "retrying 1/5", "retrying 2/5", ...

nit: maybe worth logging ntry and num_tries, e.g. "timeout 1/5", "timeout 2/5", ...

Missing:

#include <cstdint>
#include <chrono>
#include <array>

(Noticed while trying to forward-declare netaddress.h, which didn't work because of the CServices stored in MappingResult)

Will add, thanks!

This header can be made dependency-free (while making netaddress.h a little more forward-declare-friendly) with:

diff --git a/src/netaddress.h b/src/netaddress.h
index 52fecada1c9..75d172ab334 100644
--- a/src/netaddress.h
+++ b/src/netaddress.h
@@ -31,3 +31,3 @@
  */
-enum Network {
+enum Network : int {
     /// Addresses from these networks are not publicly routable on the global Internet.
diff --git a/src/util/netif.cpp b/src/util/netif.cpp
index ad3f93b379a..c4d4c3a31c8 100644
--- a/src/util/netif.cpp
+++ b/src/util/netif.cpp
@@ -9,2 +9,3 @@
 #include <logging.h>
+#include <netaddress.h>
 #include <netbase.h>
diff --git a/src/util/netif.h b/src/util/netif.h
index 5ff473fd4f2..ab31115f0ff 100644
--- a/src/util/netif.h
+++ b/src/util/netif.h
@@ -7,5 +7,7 @@

-#include <netaddress.h>
-
 #include <optional>
+#include <vector>
+
+enum Network : int;
+class CNetAddr;

I tested to see what would happen if the forward-declare went out of sync and was met with (as I'd hoped) an error:

./netaddress.h:32:6: error: enumeration redeclared with different underlying type 'int' (was 'short')
   32 | enum Network : int {
      |      ^
./util/netif.h:11:6: note: previous declaration is here
   11 | enum Network : short;

i don't disagree with doing this, but netif.h is only included in two places: net.cpp and pcp.cpp, both of which use netaddress.h anyway. Not sure it's worth adding a change in netaddress.h in this PR for.

From RFC 6886 3.2:

Upon receiving a response packet, the client MUST check the source IP address, and silently discard the packet if the address is not the address of the gateway to which the request was sent.

Using connect(2) on UDP socket nicely ensures that we only receive packets from that address (dest_addr), so that we don't have to check explicitly. Maybe expand the comment to show this purpose too and to avoid this Connect() being removed by a future refactor:

// Associate UDP socket to gateway. This used to get our local address toward the gateway and
// to ensure that we only process packets from the gateway, which is mandated by RFC 6886 3.2.

Could use the constants instead of magic numbers:

            if (response[NATPMP_HDR_VERSION_OFS] != NATPMP_VERSION || response[NATPMP_HDR_OP_OFS] != (NATPMP_RESPONSE | NATPMP_OP_MAP_TCP)) {

From https://www.rfc-editor.org/rfc/rfc6886#section-3.1:

... client sends its request packet to port 5351 of its configured gateway address, and waits 250 ms for a response. If no NAT-PMP response is received from the gateway after 250 ms, the client retransmits its request and waits 500 ms. The client SHOULD repeat this process with the interval between attempts doubling each time. If, after sending its ninth attempt (and then waiting for 64 seconds), the client has still received no response, then it SHOULD conclude that this gateway does not support NAT Port Mapping Protocol

The code is not doing as prescribed - it will wait equal amount of time after each send. I guess this is fine as well, but I wonder if there is a specific reason not to implement it as prescribed?

Edit: PCP describes a different retransmission logic: https://www.rfc-editor.org/rfc/rfc6887#section-8.1.1 (a bit convoluted IMO).

No, this doesn't implement the exact retry logic, it seems excessively complicated for communicating with a local router, and re-using the same code for NAT-PMP and PCP keeps the code straighforward.

I wonder why the difference with NATPMPRequestPortMap() where we Connect() (without Bind()) and retrieve the local address from the socket.

My understanding is that both NAT-PMP and PCP servers will reject mapping requests for one address coming from another address.

NATPMP is IPv4 only. In the case of IPv4 we don't need to explicitly bind. We don't do this for PCP either (hence passing the BIND_ANY address to bind, effectively a NO-OP). For IPv6 we know what internal address we want, and what external address we want so it's different.

(FWIW, this kind of confusion is exactly why i was hestitant to also implement this for IPv4 in the beginning, NAT and pinholing are basically two wildly different cases, which happen to share code)

I admit I am somewhat confused.

Do we assume that for IPv6, address translation is never done (e.g. https://www.rfc-editor.org/rfc/rfc6296) and that for IPv6 we only need to pinhole the firewall? Edit: apparently yes, since we only process IsRoutable() addresses returned by GetLocalAddresses().

For IPv6 we know what internal address we want, and what external address we want

You mean that both internal and external are the same and we get that from GetLocalAddresses()?

Maybe worth logging bind.ToStringAddr() here and gateway.ToStringAddr() below if Connect() fails.

Correlating responses to requests is at odds with the description in section 6:

... the message flows can be viewed as two somewhat independent streams ... These two message flows are loosely correlated ... The PCP server can send unsolicited responses to clients ... This design goal helps explain why PCP request and response messages have no transaction ID ...

According to the last paragraph in section 11.2 the nonce is used to distinguish different servers and to do some lame form of authentication :disappointed:. We use the same nonce for all requests to the same server.

I guess just update the comment to omit "to be able to correlate requests and responses".

Yes, i initially had the wrong idea of what it did, should update the comment.