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

pull laanwj wants to merge 10 commits into bitcoin:master from laanwj:2024-05-pcp changing 38 files +1049 −283
  1. laanwj commented at 6:35 pm on May 5, 2024: member

    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

    Things to consider for follow-up PRs

    • #30043 (review) avoid unreachable nets (not given to -onlynet=)

    • #30043 (review) could announce an addr:port where we do not listen (no -bind)

    • #30043 (review) could announce the wrong port because it uses GetListenPort()

    • #30043 (review) if we requested one port but another was assigned, then which one to use in the renewal?

    • #30043 (review) Use GetAdapterAddresses to discover local addresses for Windows #31014

    • Unit testing: the code is set up to use Sock to support testing harnesses, but none have been written yet. It will also be necessary to createa a mockable steady_clock for this. #31022

  2. laanwj added the label P2P on May 5, 2024
  3. laanwj added the label Needs CMake port on May 5, 2024
  4. DrahtBot commented at 6:35 pm on May 5, 2024: contributor

    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.

    Type Reviewers
    ACK vasild, Sjors, achow101
    Concept ACK theuni, fjahr

    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:

    • #30997 (build: Switch to Qt 6 by hebasto)
    • #30975 (Add multiprocess binaries to release build by Sjors)
    • #30935 (ci: Approximate MAKEJOBS in image build phase by maflcko)
    • #30634 (ci: Use clang-19 from apt.llvm.org by maflcko)
    • #30315 (Stratum v2 Transport by Sjors)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #29346 (Stratum v2 Noise Protocol by Sjors)

    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.

  5. laanwj force-pushed on May 5, 2024
  6. DrahtBot added the label CI failed on May 5, 2024
  7. DrahtBot commented at 6:49 pm on May 5, 2024: contributor

    🚧 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

  8. laanwj force-pushed on May 5, 2024
  9. laanwj force-pushed on May 5, 2024
  10. laanwj force-pushed on May 5, 2024
  11. laanwj added the label Needs release note on May 5, 2024
  12. Sjors commented at 11:08 am on May 6, 2024: member
    Concept ACK
  13. laanwj force-pushed on May 6, 2024
  14. in src/init.cpp:783 in a2d67c320f outdated
    756@@ -760,8 +757,8 @@ void InitParameterInteraction(ArgsManager& args)
    757         // do not map ports or try to retrieve public IP when not listening (pointless)
    758         if (args.SoftSetBoolArg("-upnp", false))
    759             LogPrintf("%s: parameter interaction: -listen=0 -> setting -upnp=0\n", __func__);
    760-        if (args.SoftSetBoolArg("-natpmp", false)) {
    


    Sjors commented at 12:53 pm on May 6, 2024:
    a2d67c320f8a28da98e6c8352bd67648a9b831a8: I think you need to keep this (and above) until -natpmp is removed.

    laanwj commented at 8:59 am on May 7, 2024:
    Yes, good point.
  15. theuni commented at 4:49 pm on May 6, 2024: member

    Whoa :)

    (Concept ACK)

  16. Sjors commented at 4:51 pm on May 6, 2024: member

    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.

  17. theuni commented at 6:37 pm on May 6, 2024: member

    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.

  18. DrahtBot added the label Needs rebase on May 7, 2024
  19. laanwj force-pushed on May 7, 2024
  20. laanwj commented at 8:59 am on May 7, 2024: member
    Rebased for #29984
  21. laanwj commented at 10:18 am on May 7, 2024: member

    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:

    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.

  22. DrahtBot removed the label Needs rebase on May 7, 2024
  23. in src/util/pcp.cpp:83 in fe53862fd4 outdated
    78+std::optional<MappingResult> PCPRequestPortMap(const PCPMappingNonce &nonce, const CNetAddr &gateway, const CNetAddr &bind, uint16_t port, uint32_t lifetime, int num_tries, bool option_prefer_failure)
    79+{
    80+    struct sockaddr_storage dest_addr, bind_addr;
    81+    socklen_t dest_addrlen = sizeof(struct sockaddr_storage), bind_addrlen = sizeof(struct sockaddr_storage);
    82+
    83+    LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "pcp: Requesting port mapping for addr %s port %d from gateway %s\n", bind.ToStringAddr(), port, gateway.ToStringAddr());
    


    davidgumberg commented at 4:25 pm on May 7, 2024:

    nit:

    0    LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "pcp: Requesting port mapping for addr %s port %d from gateway %s\n", bind.ToStringAddr(), port, gateway.ToStringAddr());
    
  24. luke-jr commented at 3:26 pm on May 8, 2024: member
    Would prefer this in two steps (add PCP, then remove NAT-PMP)
  25. laanwj commented at 7:25 am on May 9, 2024: member

    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.

  26. in src/qt/optionsdialog.cpp:111 in 73037f27fc outdated
    107@@ -108,9 +108,7 @@ OptionsDialog::OptionsDialog(QWidget* parent, bool enableWallet)
    108 #ifndef USE_UPNP
    109     ui->mapPortUpnp->setEnabled(false);
    110 #endif
    111-#ifndef USE_NATPMP
    112-    ui->mapPortNatpmp->setEnabled(false);
    113-#endif
    114+    ui->mapPortPCP->setEnabled(false);
    


    Sjors commented at 8:29 am on May 9, 2024:
    73037f27fc21765414c298b171dfdeee130c549b: needs to be true, but you can actually drop this line.

    laanwj commented at 7:28 am on May 16, 2024:
    Whoops, yes, it’s setEnabled, not setting the default value 😅 .
  27. in src/qt/forms/optionsdialog.ui:331 in 73037f27fc outdated
    325@@ -326,12 +326,12 @@
    326         </widget>
    327        </item>
    328        <item>
    329-        <widget class="QCheckBox" name="mapPortNatpmp">
    330+        <widget class="QCheckBox" name="mapPortPCP">
    331          <property name="toolTip">
    332-          <string>Automatically open the Bitcoin client port on the router. This only works when your router supports NAT-PMP and it is enabled. The external port could be random.</string>
    333+          <string>Automatically open the Bitcoin client port on the router. This only works when your router supports PCP and it is enabled. The external port could be random.</string>
    


    Sjors commented at 8:34 am on May 9, 2024:
    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.
  28. Sjors commented at 9:56 am on May 9, 2024: member

    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.

  29. Sjors commented at 1:53 pm on May 9, 2024: member

    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.

  30. in src/mapport.cpp:92 in 6cf4809c6b outdated
    131-                    AddLocal(external, LOCAL_MAPPED);
    132-                    external_ip_discovered = true;
    133+        // IPv6
    134+        std::optional<CNetAddr> gateway6 = QueryDefaultGateway(NET_IPV6);
    135+        if (!gateway6) {
    136+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "pcp: Could not determine IPv6 default gateway\n");
    


    Sjors commented at 3:10 pm on May 9, 2024:
    6cf4809c6b93e1720dfdfe4e3320cfd8939686b6: This is ::Warning worthy.

    laanwj commented at 7:13 am on May 16, 2024:
    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.

    laanwj commented at 7:33 am on May 16, 2024:
    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.
  31. Sjors commented at 3:45 pm on May 9, 2024: member

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

  32. DrahtBot added the label Needs rebase on May 15, 2024
  33. laanwj commented at 12:00 pm on May 15, 2024: member

    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.

  34. laanwj commented at 6:56 am on May 16, 2024: member

    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.

  35. laanwj force-pushed on May 16, 2024
  36. laanwj commented at 7:43 am on May 16, 2024: member
    • 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.
  37. laanwj force-pushed on May 16, 2024
  38. laanwj force-pushed on May 16, 2024
  39. laanwj force-pushed on May 16, 2024
  40. DrahtBot removed the label CI failed on May 16, 2024
  41. DrahtBot removed the label Needs rebase on May 16, 2024
  42. in src/util/netif.cpp:24 in 39933a53f3 outdated
    14+
    15+#ifdef HAVE_LINUX_ROUTE_H
    16+
    17+#include <linux/route.h>
    18+
    19+std::optional<CNetAddr> QueryDefaultGateway(Network network)
    


    vasild commented at 3:10 pm on May 16, 2024:

    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:

      0--- a/src/test/netbase_tests.cpp
      1+++ b/src/test/netbase_tests.cpp
      2@@ -1,25 +1,34 @@
      3 // Copyright (c) 2012-2022 The Bitcoin Core developers
      4 // Distributed under the MIT software license, see the accompanying
      5 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
      6 
      7+#include <compat/compat.h>
      8 #include <net_permissions.h>
      9 #include <netaddress.h>
     10 #include <netbase.h>
     11 #include <netgroup.h>
     12 #include <protocol.h>
     13 #include <serialize.h>
     14 #include <streams.h>
     15 #include <test/util/setup_common.h>
     16 #include <util/strencodings.h>
     17+#include <util/syserror.h>
     18 #include <util/translation.h>
     19 
     20 #include <string>
     21 
     22 #include <boost/test/unit_test.hpp>
     23 
     24+#ifdef __linux__
     25+#include <linux/rtnetlink.h>
     26+#elif defined(__FreeBSD__)
     27+#include <netlink/netlink.h>
     28+#include <netlink/netlink_route.h>
     29+#endif
     30+
     31 using namespace std::literals;
     32 
     33 BOOST_FIXTURE_TEST_SUITE(netbase_tests, BasicTestingSetup)
     34 
     35 static CNetAddr ResolveIP(const std::string& ip)
     36 {
     37@@ -610,7 +619,100 @@ BOOST_AUTO_TEST_CASE(isbadport)
     38             ++total_bad_ports;
     39         }
     40     }
     41     BOOST_CHECK_EQUAL(total_bad_ports, 80);
     42 }
     43 
     44+std::optional<CNetAddr> QueryDefaultGateway(Network network)
     45+{
     46+    Assume(network == NET_IPV4 || network == NET_IPV6);
     47+
     48+    // Create a netlink socket.
     49+
     50+    const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
     51+    if (s < 0) {
     52+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", SysErrorString(errno));
     53+        return std::nullopt;
     54+    }
     55+    Sock sock{static_cast<SOCKET>(s)};
     56+
     57+    // Send request.
     58+
     59+    struct {
     60+        nlmsghdr hdr; ///< Request header.
     61+        rtmsg data; ///< Request data, a "route message".
     62+        nlattr dst_hdr; ///< One attribute, conveying the route destination address.
     63+        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.
     64+    } request;
     65+
     66+    // Whether to use the first 4 or 16 bytes from request.attr_dst_data.
     67+    const size_t dst_data_len = network == NET_IPV4 ? 4 : 16;
     68+
     69+    memset(&request, 0x0, sizeof(request));
     70+
     71+    request.hdr.nlmsg_type = RTM_GETROUTE;
     72+    request.hdr.nlmsg_flags = NLM_F_REQUEST;
     73+#ifdef __linux__
     74+    request.hdr.nlmsg_flags |= NLM_F_DUMP;
     75+#endif
     76+    request.hdr.nlmsg_len = NLMSG_LENGTH(sizeof(rtmsg) + sizeof(nlattr) + dst_data_len);
     77+    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.
     78+    request.data.rtm_family = network == NET_IPV4 ? AF_INET : AF_INET6;
     79+    request.data.rtm_dst_len = 0; // Prefix length.
     80+#ifdef __FreeBSD__
     81+    request.data.rtm_flags = RTM_F_PREFIX;
     82+#endif
     83+    request.dst_hdr.nla_type = RTA_DST;
     84+    request.dst_hdr.nla_len = sizeof(nlattr) + dst_data_len;
     85+
     86+    if (sock.Send(&request, sizeof(request), 0) != sizeof(request)) {
     87+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "send() to netlink socket: %s\n", SysErrorString(errno));
     88+        return std::nullopt;
     89+    }
     90+
     91+    // Receive response.
     92+
     93+    char response[4096];
     94+    ssize_t response_len;
     95+    do {
     96+        response_len = sock.Recv(response, sizeof(response), 0);
     97+    } while (response_len < 0 && (errno == EINTR || errno == EAGAIN));
     98+    if (response_len < 0) {
     99+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "recv() from netlink socket: %s\n", SysErrorString(errno));
    100+        return std::nullopt;
    101+    }
    102+
    103+    for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {
    104+        rtmsg* r = (rtmsg*)NLMSG_DATA(hdr);
    105+        int remaining_len = RTM_PAYLOAD(hdr);
    106+        // Iterate over the attributes.
    107+        for (rtattr* attr = RTM_RTA(r); RTA_OK(attr, remaining_len); attr = RTA_NEXT(attr, remaining_len)) {
    108+            if (attr->rta_type == RTA_GATEWAY) {
    109+                if (network == NET_IPV4) {
    110+                    Assume(sizeof(in_addr) == RTA_PAYLOAD(attr));
    111+                    return CNetAddr{in_addr{.s_addr = *static_cast<decltype(in_addr::s_addr)*>(RTA_DATA(attr))}};
    112+                } else {
    113+                    Assume(sizeof(in6_addr) == RTA_PAYLOAD(attr));
    114+                    in6_addr gw;
    115+                    std::memcpy(&gw, RTA_DATA(attr), sizeof(gw));
    116+                    return CNetAddr{gw};
    117+                }
    118+            }
    119+        }
    120+    }
    121+
    122+    return std::nullopt;
    123+}
    124+
    125+BOOST_AUTO_TEST_CASE(netlink)
    126+{
    127+    for (const auto net : {NET_IPV4, NET_IPV6}) {
    128+        const auto gw{QueryDefaultGateway(net)};
    129+        if (gw.has_value()) {
    130+            printf("Default %s gateway: %s\n", GetNetworkName(net).c_str(), gw->ToStringAddr().c_str());
    131+        } else {
    132+            printf("No %s default gateway.\n", GetNetworkName(net).c_str());
    133+        }
    134+    }
    135+}
    136+
    137 BOOST_AUTO_TEST_SUITE_END()
    

    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


    laanwj commented at 5:31 pm on May 16, 2024:
    Huh interesting. i didn’t know netlink worked for multiple operating systems, that’s much better.

    laanwj commented at 6:37 pm on May 16, 2024:

    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


    vasild commented at 9:30 am on May 17, 2024:

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

      0#include <arpa/inet.h>
      1#include <assert.h>
      2#include <errno.h>
      3#include <net/if.h>
      4#include <netinet/in.h>
      5#include <stdio.h>
      6#include <stdlib.h>
      7#include <string.h>
      8#include <sys/socket.h>
      9#include <sys/types.h>
     10#include <unistd.h>
     11
     12#ifdef __linux__
     13#include <linux/rtnetlink.h>
     14#elif defined(__FreeBSD__)
     15#include <netlink/netlink.h>
     16#include <netlink/netlink_route.h>
     17#endif
     18
     19void QueryDefaultGateway(sa_family_t family)
     20{
     21    // Create a netlink socket.
     22
     23    int sock = socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE);
     24    if (sock < 0) {
     25        perror("socket(AF_NETLINK)");
     26        return;
     27    }
     28
     29    // Send request.
     30
     31    struct {
     32        nlmsghdr hdr; ///< Request header.
     33        rtmsg data; ///< Request data, a "route message".
     34        nlattr dst_hdr; ///< One attribute, conveying the route destination address.
     35        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.
     36    } request{};
     37
     38    // Whether to use the first 4 or 16 bytes from request.attr_dst_data.
     39    const size_t dst_data_len = family == AF_INET ? 4 : 16;
     40
     41    request.hdr.nlmsg_type = RTM_GETROUTE;
     42    request.hdr.nlmsg_flags = NLM_F_REQUEST;
     43#ifdef __linux__
     44    // XXX some strange behavior:
     45    // Linux IPv4 - this must be present, otherwise no gateway is found
     46    // Linux IPv6 - this must be present, otherwise no gateway is found
     47    // FreeBSD IPv4 - does not matter, the gateway is found with or without this
     48    // FreeBSD IPv6 - this must be absent, otherwise no gateway is found
     49    request.hdr.nlmsg_flags |= NLM_F_DUMP;
     50#endif
     51    request.hdr.nlmsg_len = NLMSG_LENGTH(sizeof(rtmsg) + sizeof(nlattr) + dst_data_len);
     52    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.
     53    request.data.rtm_family = family;
     54    request.data.rtm_dst_len = 0; // Prefix length.
     55    //request.data.rtm_table = RT_TABLE_MAIN;
     56    //request.data.rtm_protocol = RTPROT_STATIC;
     57    //request.data.rtm_scope = RT_SCOPE_UNIVERSE;
     58    //request.data.rtm_type = RTN_UNICAST;
     59#ifdef __FreeBSD__
     60    // XXX some strange behavior:
     61    // Linux IPv4 - this must be absent, otherwise no gateway is found
     62    // Linux IPv6 - this must be absent, otherwise no gateway is found
     63    // FreeBSD IPv4 - does not matter, the gateway is found with or without this
     64    // FreeBSD IPv6 - this must be present, otherwise no gateway is found
     65    request.data.rtm_flags = RTM_F_PREFIX;
     66#endif
     67    request.dst_hdr.nla_type = RTA_DST;
     68    request.dst_hdr.nla_len = sizeof(nlattr) + dst_data_len;
     69
     70    if (send(sock, &request, request.hdr.nlmsg_len, 0) != request.hdr.nlmsg_len) {
     71        fprintf(stderr, "send() failed to send %u bytes\n", request.hdr.nlmsg_len);
     72        close(sock);
     73        return;
     74    }
     75
     76    // Receive response.
     77
     78    char response[4096];
     79    ssize_t response_len;
     80    do {
     81        response_len = recv(sock, response, sizeof(response), 0);
     82    } while (response_len < 0 && (errno == EINTR || errno == EAGAIN));
     83    if (response_len < 0) {
     84        fprintf(stderr, "recv(): %s\n", strerror(errno));
     85        close(sock);
     86        return;
     87    }
     88
     89    for (nlmsghdr *hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {
     90        rtmsg* r = (rtmsg*)NLMSG_DATA(hdr);
     91        int remaining_len = RTM_PAYLOAD(hdr);
     92
     93        // Iterate over the attributes.
     94        rtattr* rta_gateway = nullptr;
     95        int scope_id = 0;
     96        for (rtattr *attr = RTM_RTA(r); RTA_OK(attr, remaining_len); attr = RTA_NEXT(attr, remaining_len)) {
     97            if (attr->rta_type == RTA_GATEWAY) {
     98                rta_gateway = attr;
     99            } else if (attr->rta_type == RTA_OIF) {
    100                assert(sizeof(int) == RTA_PAYLOAD(attr));
    101                memcpy(&scope_id, RTA_DATA(attr), sizeof(scope_id));
    102            }
    103        }
    104
    105        // Found gateway?
    106        if (rta_gateway != nullptr) {
    107            char buf[256];
    108            printf("%s gateway: %s",
    109                   family == AF_INET ? "IPv4" : "IPv6",
    110                   inet_ntop(r->rtm_family, RTA_DATA(rta_gateway), buf, sizeof(buf)));
    111            if (family == AF_INET6) {
    112                printf(", scope id: %d\n", scope_id);
    113            } else {
    114                printf("\n");
    115            }
    116        }
    117    }
    118
    119    close(sock);
    120}
    121
    122int main(int argc, char** argv)
    123{
    124    QueryDefaultGateway(AF_INET);
    125    QueryDefaultGateway(AF_INET6);
    126    return 0;
    127}
    

    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?

  43. vasild commented at 3:48 pm on May 16, 2024: contributor
    Concept ACK
  44. laanwj commented at 7:04 pm on May 16, 2024: member

    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.

  45. DrahtBot commented at 8:53 pm on May 16, 2024: contributor

    🚧 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

  46. DrahtBot added the label CI failed on May 16, 2024
  47. laanwj force-pushed on May 17, 2024
  48. laanwj commented at 8:18 am on May 17, 2024: member

    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
    
  49. laanwj force-pushed on May 17, 2024
  50. in src/util/netif.cpp:63 in 841deffd44 outdated
    58+#endif
    59+    request.dst_hdr.nla_type = RTA_DST;
    60+    request.dst_hdr.nla_len = sizeof(nlattr) + dst_data_len;
    61+
    62+    if (sock.Send(&request, sizeof(request), 0) != sizeof(request)) {
    63+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "send() to netlink socket: %s\n", SysErrorString(errno));
    


    vasild commented at 9:56 am on May 17, 2024:

    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:

    0    if (sock.Send(&request, request.hdr.nlmsg_len, 0) != request.hdr.nlmsg_len) {
    1        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():

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

    laanwj commented at 10:32 am on May 17, 2024:

    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.

  51. sipa commented at 2:11 pm on May 17, 2024: member

    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.

  52. laanwj force-pushed on May 17, 2024
  53. laanwj commented at 2:22 pm on May 17, 2024: member

    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.

  54. sipa commented at 2:28 pm on May 17, 2024: member
    @laanwj 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.
  55. laanwj commented at 2:35 pm on May 17, 2024: member

    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.

  56. laanwj force-pushed on May 17, 2024
  57. DrahtBot removed the label CI failed on May 17, 2024
  58. laanwj added the label DrahtBot Guix build requested on May 17, 2024
  59. laanwj removed the label DrahtBot Guix build requested on May 17, 2024
  60. laanwj force-pushed on May 17, 2024
  61. laanwj added the label DrahtBot Guix build requested on May 18, 2024
  62. laanwj force-pushed on May 18, 2024
  63. laanwj commented at 12:15 pm on May 18, 2024: member
    MacOS implementation of default gateway finding was added as well (i’ve tested it on MacOS Monterey, but could always use more, especially ARM macs would be interesting). This concludes the coverage of default gateway-finding on the major platforms.
  64. laanwj force-pushed on May 19, 2024
  65. laanwj commented at 8:09 am on May 19, 2024: member
    Squashed all fixups into their logical commit. This should be in a good state for review now. The only TODO that is left to do is the Qt settings migration (so that if the user had enabled natpmp, pcp will now be enabled), i could use some help with that because i don’t really know the new mechanism.
  66. DrahtBot added the label CI failed on May 19, 2024
  67. DrahtBot removed the label CI failed on May 20, 2024
  68. sipa commented at 11:32 pm on May 20, 2024: member

    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
    
  69. laanwj commented at 7:47 am on May 21, 2024: member

    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.

  70. sipa commented at 11:44 am on May 21, 2024: member
    @laanwj This is a router provided by Verizon (a large US ISP) in 2021.
  71. laanwj commented at 12:39 pm on May 21, 2024: member

    @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?

  72. DrahtBot commented at 5:55 am on May 22, 2024: contributor

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

    File commit a786fd2041913d82ca90b561de309421bd24e41b(master) commit 7718a5c42f086b7c78000770a68170ff83c79038(master and this pull)
    SHA256SUMS.part 14f282b5bf7ebcd4... bde34ea3844bc5a1...
    *-aarch64-linux-gnu-debug.tar.gz b2364625629230d1... 0ddcad066c06e5b5...
    *-aarch64-linux-gnu.tar.gz b05fc0561baa0966... cefac02d8a5e9782...
    *-arm-linux-gnueabihf-debug.tar.gz 4545637daf89882c... 391b99ffa321ccd8...
    *-arm-linux-gnueabihf.tar.gz 27115f4c7e251175... 46f4d4ec47bc6a05...
    *-arm64-apple-darwin-unsigned.tar.gz cf3950f9d05fb1c0... 436221ec28ee31a4...
    *-arm64-apple-darwin-unsigned.zip b1c32e004722a3b4... 8c509e13ed24a312...
    *-arm64-apple-darwin.tar.gz f09a38acef4b141b... b769aecd340fd798...
    *-powerpc64-linux-gnu-debug.tar.gz 75cfe8d73120b00d... feb0c0e88628b733...
    *-powerpc64-linux-gnu.tar.gz d4f8c1296886fac4... 2588795e780f8e6c...
    *-riscv64-linux-gnu-debug.tar.gz a8f56aee14e85f29... 2d8829895961918e...
    *-riscv64-linux-gnu.tar.gz d8a4ae4295e6aa9f... 94779be512d355c9...
    *-x86_64-apple-darwin-unsigned.tar.gz ff9ae232120c85f9... 295c1ee6c4dc4526...
    *-x86_64-apple-darwin-unsigned.zip 9917159a6fd59954... 9c28f7151880f1f4...
    *-x86_64-apple-darwin.tar.gz 61112274cace86c4... 27bf6a03f1abce44...
    *-x86_64-linux-gnu-debug.tar.gz 557d05bc585a2910... 36db8ad04f331f36...
    *-x86_64-linux-gnu.tar.gz 4928355769a494d7... 5365b31596e65699...
    *.tar.gz 49c2acd89613cf65... 51524f7ae096c36f...
    guix_build.log 88d5371b29f8bb75... d18448a1f3d9cc20...
    guix_build.log.diff 3207cbd48d885913...
  73. DrahtBot removed the label DrahtBot Guix build requested on May 22, 2024
  74. laanwj commented at 9:04 am on May 22, 2024: member

    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

  75. Sjors commented at 10:47 am on May 22, 2024: member

    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.

  76. laanwj commented at 11:34 am on May 22, 2024: member

    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)

  77. Sjors commented at 12:11 pm on May 22, 2024: member

    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.

  78. laanwj commented at 12:23 pm on May 22, 2024: member

    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.

  79. Sjors commented at 12:30 pm on May 22, 2024: member

    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.”.

  80. in src/util/netif.cpp:14 in bebfafcf98 outdated
     9+#include <logging.h>
    10+#include <netbase.h>
    11+#include <util/check.h>
    12+#include <util/sock.h>
    13+#include <util/syserror.h>
    14+
    


    Sjors commented at 1:26 pm on May 22, 2024:

    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:

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

    and end with

    0#else return std::nullopt;
    

    laanwj commented at 8:26 am on May 23, 2024:
    Agree, i think i would prefer QueryDefaultGatewayNetlink QueryDefaultGatewaySysctl QueryDefaultGatewayWin32 – only the WIN32 one is truly OS specific, the other ones are POSIX-ish

    laanwj commented at 11:40 am on May 23, 2024:
    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.
  81. in src/util/netif.cpp:17 in bebfafcf98 outdated
    14+
    15+#if defined(__linux__) || defined(__FreeBSD__)
    16+
    17+#if defined(__linux__)
    18+#include <linux/rtnetlink.h>
    19+#elif defined(__FreeBSD__)
    


    Sjors commented at 2:56 pm on May 22, 2024:
    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.

    Sjors commented at 3:28 pm on May 22, 2024:

    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:

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


    laanwj commented at 4:19 pm on May 22, 2024:
    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.

    Sjors commented at 4:27 pm on May 22, 2024:

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

    Seems fine to me.


    laanwj commented at 4:42 pm on May 22, 2024:
    Added a version check for at least 13.2.

    vasild commented at 3:26 pm on May 28, 2024:
    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.

    laanwj commented at 5:28 pm on May 28, 2024:
    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.

    sipa commented at 7:08 pm on May 28, 2024:
    @laanwj One possibility is moving the code to a separate .c file, perhaps?

    laanwj commented at 7:34 pm on May 28, 2024:
    That would probably work, but that’d be a really invasive workaround build system wise.

    theuni commented at 8:32 pm on May 28, 2024:
    Yes, please don’t do that :)

    laanwj commented at 9:31 pm on May 28, 2024:
    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).

    vasild commented at 10:26 am on May 29, 2024:

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

    The suggested workaround to define typeof is ok:

     0diff --git i/src/util/netif.cpp w/src/util/netif.cpp
     1index 845b8aed1d..1840974a83 100644
     2--- i/src/util/netif.cpp
     3+++ w/src/util/netif.cpp
     4@@ -9,18 +9,24 @@
     5 #include <logging.h>
     6 #include <netbase.h>
     7 #include <util/check.h>
     8 #include <util/sock.h>
     9 #include <util/syserror.h>
    10 
    11+#ifdef __FreeBSD__
    12+#include <osreldate.h>
    13+#endif
    14+
    15 // Linux and FreeBSD 13.2+
    16-#if defined(__linux__) || (defined(__FreeBSD__) && __FreeBSD_version >= 1302000)
    17+#if defined(__linux__) || __FreeBSD_version >= 1302000
    18 
    19 #if defined(__linux__)
    20 #include <linux/rtnetlink.h>
    21 #elif defined(__FreeBSD__)
    22+// Workaround https://github.com/freebsd/freebsd-src/pull/1070.
    23+#define typeof __typeof
    24 #include <netlink/netlink.h>
    25 #include <netlink/netlink_route.h>
    26 #endif
    27 
    28 static std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
    29 {
    30@@ -77,14 +83,13 @@ static std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
    31     } while (recv_result < 0 && (errno == EINTR || errno == EAGAIN));
    32     if (recv_result < 0) {
    33         LogPrintLevel(BCLog::NET, BCLog::Level::Error, "recv() from netlink socket: %s\n", NetworkErrorString(errno));
    34         return std::nullopt;
    35     }
    36 
    37-    size_t response_len = static_cast<size_t>(recv_result);
    38-    for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, response_len); hdr = NLMSG_NEXT(hdr, response_len)) {
    39+    for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) {
    40         rtmsg* r = (rtmsg*)NLMSG_DATA(hdr);
    41         int remaining_len = RTM_PAYLOAD(hdr);
    42 
    43         // Iterate over the attributes.
    44         rtattr *rta_gateway = nullptr;
    45         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.


    laanwj commented at 10:33 am on May 29, 2024:

    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.


    vasild commented at 12:36 pm on May 29, 2024:

    On my Linux distro I have:

    0struct nlmsghdr {          
    1        __u32           nlmsg_len;
    2...
    3#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
    4                           (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
    5                           (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:

    0    ssize_t response_len;
    1...
    2    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?


    laanwj commented at 1:10 pm on May 29, 2024:

    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.


    vasild commented at 3:51 pm on May 29, 2024:
    I see, then defining recv_result as int64_t instead of ssize_t should be ok on all platforms?

    laanwj commented at 7:28 am on May 30, 2024:
    Will try. Introducing a platform depenendent typedef just for this would be a bit silly.
  82. laanwj force-pushed on May 22, 2024
  83. laanwj renamed this:
    net: Replace libnatpmp with built-in PCP implementation
    net: Replace libnatpmp with built-in PCP+NATPMP implementation
    on May 22, 2024
  84. laanwj commented at 4:21 pm on May 22, 2024: member
    Okay - added a NAT-PMP fallback and removed all user visible run-time option changes, except for mentioning PCP in documentation.
  85. in src/init.cpp:559 in bd2cc38d8a outdated
    555@@ -556,11 +556,7 @@ void SetupServerArgs(ArgsManager& argsman)
    556 #else
    557     hidden_args.emplace_back("-upnp");
    558 #endif
    559-#ifdef USE_NATPMP
    560-    argsman.AddArg("-natpmp", strprintf("Use NAT-PMP to map the listening port (default: %u)", DEFAULT_NATPMP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    561-#else
    562-    hidden_args.emplace_back("-natpmp");
    563-#endif // USE_NATPMP
    564+    argsman.AddArg("-natpmp", strprintf("Use PCP or NATPMP to map the listening port (default: %u)", DEFAULT_NATPMP), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
    


    Sjors commented at 4:30 pm on May 22, 2024:

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

    (you lost the -)


    laanwj commented at 4:33 pm on May 22, 2024:
    Whoops, fixed
  86. DrahtBot added the label CI failed on May 22, 2024
  87. DrahtBot commented at 4:31 pm on May 22, 2024: contributor

    🚧 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

  88. in src/util/netif.cpp:31 in 82b0bff51f outdated
    26+    Assume(network == NET_IPV4 || network == NET_IPV6);
    27+
    28+    // Create a netlink socket.
    29+    const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
    30+    if (s < 0) {
    31+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", SysErrorString(errno));
    


    Sjors commented at 6:11 pm on May 22, 2024:
    82b0bff51fedff2ce5e47cae1c75b9172766d8ef: the SysErrorString documentation says you should call NetworkErrorString, though that will in turn just call SysErrorString, since this is never called under WIN32.

    laanwj commented at 7:32 pm on May 22, 2024:
    Right, will update this (though yeah for POSIX operating systems there’s no difference).
  89. in src/util/netif.cpp:65 in 82b0bff51f outdated
    45+    const size_t dst_data_len = network == NET_IPV4 ? 4 : 16;
    46+
    47+    request.hdr.nlmsg_type = RTM_GETROUTE;
    48+    request.hdr.nlmsg_flags = NLM_F_REQUEST;
    49+#ifdef __linux__
    50+    request.hdr.nlmsg_flags |= NLM_F_DUMP;
    


    Sjors commented at 6:28 pm on May 22, 2024:
    82b0bff51fedff2ce5e47cae1c75b9172766d8ef Why only on linux? It seems to exist on FreeBSD too: https://man.freebsd.org/cgi/man.cgi?netlink(4)

    laanwj commented at 7:35 pm on May 22, 2024:
    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.

    Sjors commented at 7:58 pm on May 22, 2024:

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

    0    // Linux IPv4 / IPv6 - this must be present, otherwise no gateway is found
    1    // FreeBSD IPv4 - does not matter, the gateway is found with or without this
    2    // FreeBSD IPv6 - this must be absent, otherwise no gateway is found
    3    request.hdr.nlmsg_flags |= NLM_F_DUMP;
    4
    5#ifdef __FreeBSD__
    6    // Linux IPv4 / IPv6 this must be absent, otherwise no gateway is found
    7    // FreeBSD IPv4 - does not matter, the gateway is found with or without this
    8    // FreeBSD IPv6 - this must be present, otherwise no gateway is found
    9    request.data.rtm_flags = RTM_F_PREFIX;
    

    laanwj commented at 8:18 pm on May 22, 2024:
    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.
  90. in src/util/netif.cpp:99 in 82b0bff51f outdated
    84+        rtattr *rta_gateway = nullptr;
    85+        int scope_id = 0;
    86+        for (rtattr* attr = RTM_RTA(r); RTA_OK(attr, remaining_len); attr = RTA_NEXT(attr, remaining_len)) {
    87+            if (attr->rta_type == RTA_GATEWAY) {
    88+                rta_gateway = attr;
    89+            } else if (attr->rta_type == RTA_OIF) {
    


    Sjors commented at 6:51 pm on May 22, 2024:

    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.


    Sjors commented at 7:03 pm on May 22, 2024:
    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)

    laanwj commented at 7:26 pm on May 22, 2024:
    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).

    laanwj commented at 7:29 pm on May 22, 2024:

    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


    Sjors commented at 7:46 pm on May 22, 2024:

    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


    Sjors commented at 7:54 pm on May 22, 2024:

    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/

  91. Sjors commented at 6:56 pm on May 22, 2024: member
    Took a cursory look at the linux / bsd approach.
  92. laanwj force-pushed on May 22, 2024
  93. DrahtBot removed the label CI failed on May 23, 2024
  94. sipa commented at 2:11 am on May 23, 2024: member

    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)

  95. laanwj commented at 6:49 am on May 23, 2024: member

    Success! My node is reachable publicly, without configuration.

    Nice!!! Thanks for testing again.

  96. in src/util/netif.cpp:139 in 82b0bff51f outdated
    134+        return std::nullopt;
    135+    }
    136+
    137+    status = GetBestInterfaceEx((sockaddr*)&destination_address, &best_if_idx);
    138+    if (status != NO_ERROR) {
    139+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Could not get best interface for default route: %s\n", SysErrorString(status));
    


    Sjors commented at 8:36 am on May 23, 2024:

    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.


    Sjors commented at 8:37 am on May 23, 2024:
    That said, this Windows code is a lot simpler than the Linux stuff above (ducks…).

    laanwj commented at 8:40 am on May 23, 2024:
    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.

    laanwj commented at 8:49 am on May 23, 2024:

    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.

  97. in src/util/netif.cpp:153 in 82b0bff51f outdated
    148+                best_if_idx, SysErrorString(status));
    149+        return std::nullopt;
    150+    }
    151+
    152+    if (network == NET_IPV4) {
    153+        Assume(best_route.NextHop.si_family == AF_INET);
    


    Sjors commented at 9:29 am on May 23, 2024:
    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.

    laanwj commented at 10:18 am on May 23, 2024:

    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.


    laanwj commented at 10:30 am on May 23, 2024:

    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

  98. in src/util/netif.cpp:188 in 82b0bff51f outdated
    183+    // MacOS: Get default gateway from route table.
    184+    // See man page for route(4) for the format.
    185+    Assume(network == NET_IPV4 || network == NET_IPV6);
    186+    int family;
    187+    if (network == NET_IPV4) {
    188+        family = AF_INET;
    


    Sjors commented at 10:17 am on May 23, 2024:
    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.

    laanwj commented at 10:23 am on May 23, 2024:
    i prefer using constants to string based APIs, if given the choice. This avoids say, typos.

    laanwj commented at 10:47 am on May 23, 2024:

    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

  99. laanwj force-pushed on May 23, 2024
  100. in src/util/netif.cpp:185 in 89ac09777d outdated
    180+    // See man page for route(4) for the format.
    181+
    182+    // net.route.0.inet[6].flags.gateway
    183+    int mib[] = {CTL_NET, PF_ROUTE, 0, family, NET_RT_FLAGS, RTF_GATEWAY};
    184+    size_t l = 0;
    185+    if (sysctl(mib, sizeof(mib) / sizeof(int), 0, &l, 0, 0) < 0) {
    


    Sjors commented at 12:22 pm on May 23, 2024:

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

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

    laanwj commented at 2:43 pm on May 23, 2024:

    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.


    Sjors commented at 3:19 pm on May 23, 2024:
    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.
  101. in src/util/netif.cpp:190 in 89ac09777d outdated
    185+    if (sysctl(mib, sizeof(mib) / sizeof(int), 0, &l, 0, 0) < 0) {
    186+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Could not get sysctl length of routing table: %s\n", SysErrorString(errno));
    187+        return std::nullopt;
    188+    }
    189+    std::vector<std::byte> buf(l);
    190+    if (sysctl(mib, sizeof(mib) / sizeof(int), buf.data(), &l, 0, 0) < 0) {
    


    Sjors commented at 12:22 pm on May 23, 2024:
    0if (sysctl(/*name=*/mib, /*namelen=*/sizeof(mib) / sizeof(int), /*oldp=*/buf.data(), /*oldlenp=*/&l, /*newp=*/nullptr, /*newlen=*/0) < 0) {
    
  102. in src/util/netif.cpp:193 in 89ac09777d outdated
    179+    // MacOS: Get default gateway from route table.
    180+    // See man page for route(4) for the format.
    181+
    182+    // net.route.0.inet[6].flags.gateway
    183+    int mib[] = {CTL_NET, PF_ROUTE, 0, family, NET_RT_FLAGS, RTF_GATEWAY};
    184+    size_t l = 0;
    


    Sjors commented at 12:24 pm on May 23, 2024:

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

    0    // The size of the available data can be determined by calling sysctl() with
    1    // the NULL argument for oldp. See sysctl(3).
    
  103. in src/util/netif.cpp:195 in 89ac09777d outdated
    190+    if (sysctl(mib, sizeof(mib) / sizeof(int), buf.data(), &l, 0, 0) < 0) {
    191+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Could not get sysctl data of routing table: %s\n", SysErrorString(errno));
    192+        return std::nullopt;
    193+    }
    194+    const struct rt_msghdr* rt = nullptr;
    195+    for (const std::byte* p = buf.data(); p < buf.data() + buf.size(); p += rt->rtm_msglen) {
    


    Sjors commented at 12:49 pm on May 23, 2024:
    I guess you can’t do something closer to for (const struct rt_msghdr* rt : buf) (somehow passing in rt->rtm_msglen).

    laanwj commented at 2:42 pm on May 23, 2024:
    i don’t think so, the pointer needs to advance by the size of the specific message, which is part of that message.

    Sjors commented at 3:58 pm on May 23, 2024:

    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.

    0    std::byte* p = buf.data();
    1    while (true) {
    2        rt = (const struct rt_msghdr*)p;
    3        // ...
    4
    5        p += rt->rtm_msglen;
    6        if (p == buf.data() + buf.size()) break;
    7        assert(p < buf.data() + buf.size());
    8    }
    
  104. in src/util/netif.cpp:197 in 89ac09777d outdated
    192+        return std::nullopt;
    193+    }
    194+    const struct rt_msghdr* rt = nullptr;
    195+    for (const std::byte* p = buf.data(); p < buf.data() + buf.size(); p += rt->rtm_msglen) {
    196+        // Iterate over routing entry addresses, get destination and gateway (if present).
    197+        rt = (const struct rt_msghdr*)p;
    


    Sjors commented at 1:07 pm on May 23, 2024:

    Do we want to check rt->rtm_errno first?

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


    laanwj commented at 2:48 pm on May 23, 2024:
    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).

    Sjors commented at 3:16 pm on May 23, 2024:
    Mmm, I thought perhaps it would return a single error entry to indicate failure.

    laanwj commented at 7:20 pm on May 23, 2024:
    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.
  105. in src/util/netif.cpp:198 in 89ac09777d outdated
    193+    }
    194+    const struct rt_msghdr* rt = nullptr;
    195+    for (const std::byte* p = buf.data(); p < buf.data() + buf.size(); p += rt->rtm_msglen) {
    196+        // Iterate over routing entry addresses, get destination and gateway (if present).
    197+        rt = (const struct rt_msghdr*)p;
    198+        const struct sockaddr* sa = (const struct sockaddr*)(rt + 1);
    


    Sjors commented at 1:22 pm on May 23, 2024:
    0// We only read from this address if a rtm_addrs bit flag is set.
    

    laanwj commented at 2:45 pm on May 23, 2024:

    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.


    Sjors commented at 3:15 pm on May 23, 2024:
    I’m not sure, but the reason I wrote this comment is because initially I thought: yikes, what if this is out of bound?

    Sjors commented at 4:10 pm on May 23, 2024:

    How about:

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

    laanwj commented at 4:48 pm on May 23, 2024:
    i moved the cast to the if() inside the inner loop.
  106. in src/util/netif.cpp:223 in 89ac09777d outdated
    202+            if (rt->rtm_addrs & (1 << i)) {
    203+                if (i == RTAX_DST) {
    204+                    dst = FromSockAddr(sa);
    205+                } else if (i == RTAX_GATEWAY) {
    206+                    gateway = FromSockAddr(sa);
    207+                }
    


    Sjors commented at 1:45 pm on May 23, 2024:
    0// Skip sockaddr entries for bit flags we're not interested in,
    1// move cursor.
    
  107. in src/util/netif.cpp:208 in 89ac09777d outdated
    203+                if (i == RTAX_DST) {
    204+                    dst = FromSockAddr(sa);
    205+                } else if (i == RTAX_GATEWAY) {
    206+                    gateway = FromSockAddr(sa);
    207+                }
    208+                sa = (const struct sockaddr*)((std::byte*)sa + ROUNDUP(sa->sa_len));
    


    Sjors commented at 1:47 pm on May 23, 2024:
    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.

    laanwj commented at 2:00 pm on May 23, 2024:

    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.


    Sjors commented at 3:14 pm on May 23, 2024:

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


    laanwj commented at 4:29 pm on May 23, 2024:

    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.


    laanwj commented at 4:39 pm on May 23, 2024:

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

     0    // Iterate over messages (each message is a routing table entry).
     1    for (size_t msgptr = 0; msgptr < buf.size(); ) {
     2        Assume((msgptr + sizeof(rt_msghdr)) <= buf.size());
     3        const struct rt_msghdr* rt = (const struct rt_msghdr*)(buf.data() + msgptr);
     4        // Iterate over addresses within entry, get destination and gateway (if present).
     5        // Pointer to address data within message, starts after header.
     6        size_t saptr = msgptr + sizeof(rt_msghdr);
     7        size_t next_msgptr = msgptr + rt->rtm_msglen;
     8        Assume(next_msgptr <= buf.size());
     9        std::optional<CNetAddr> dst;
    10        std::optional<CNetAddr> gateway;
    11        for (int i = 0; i < RTAX_MAX; i++) {
    12            if (rt->rtm_addrs & (1 << i)) {
    13                Assume((saptr + 1) <= next_msgptr);
    14                const struct sockaddr* sa = (const struct sockaddr*)(buf.data() + saptr);
    15                Assume((saptr + sa->sa_len) <= next_msgptr);
    16                if (i == RTAX_DST) {
    17                    dst = FromSockAddr(sa);
    18                } else if (i == RTAX_GATEWAY) {
    19                    gateway = FromSockAddr(sa);
    20                }
    21                // Skip to next address.
    22                saptr += sa->sa_len;
    23            }
    24        }
    25        // Found default gateway?
    26        if (dst && gateway && dst->IsBindAny()) { // Route to 0.0.0.0 or :: ?
    27            return *gateway;
    28        }
    29        // Skip to next message.
    30        msgptr = next_msgptr;
    31    }
    

    Edit: add anti-overflow assumptions.


    Sjors commented at 5:32 pm on May 23, 2024:

    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.

  108. Sjors commented at 1:50 pm on May 23, 2024: member
    Finished macOS review of QueryDefaultGatewayImpl.
  109. DrahtBot added the label CI failed on May 23, 2024
  110. DrahtBot commented at 3:08 pm on May 23, 2024: contributor

    🚧 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

  111. in src/util/pcp.cpp:233 in 82b1c25b9c outdated
    223+        if (ntry > 0) {
    224+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "%s: Retrying (%d)\n", protocol, ntry);
    225+        }
    226+        // Dispatch packet to gateway.
    227+        if (sock.Send(request.data(), request.size(), 0) != static_cast<ssize_t>(request.size())) {
    228+            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s: Could not send request: %s\n", protocol, NetworkErrorString(WSAGetLastError()));
    


    Sjors commented at 3:25 pm on May 23, 2024:

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


    Sjors commented at 3:33 pm on May 23, 2024:
    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.

    laanwj commented at 4:45 pm on May 23, 2024:

    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)


    Sjors commented at 7:46 am on May 24, 2024:
    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?

    Sjors commented at 9:36 am on May 24, 2024:

    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.

  112. laanwj force-pushed on May 23, 2024
  113. Sjors commented at 8:15 am on May 24, 2024: member

    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.

  114. in src/mapport.cpp:67 in 9a265c6d75 outdated
    85+            LogPrintLevel(BCLog::NET, BCLog::Level::Info, "portmap: Added mapping %s\n", mapping->ToString());
    86+            AddLocal(mapping->external, LOCAL_MAPPED);
    87+            ret = true;
    88+            actual_lifetime = std::min(actual_lifetime, mapping->lifetime);
    89+        } else if (MappingError *err = std::get_if<MappingError>(&res)) {
    90+            // Detailed error will already have been logged internally in respective Pormap function.
    


    Sjors commented at 10:23 am on May 24, 2024:
    typo: portmap
  115. in src/mapport.cpp:125 in 9a265c6d75 outdated
    201+            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "portmap: Got impossibly short mapping lifetime of %d seconds\n", actual_lifetime);
    202+            return false;
    203         }
    204-    }
    205+        // RFC6887 11.2.1 recommends that clients send their first renewal packet at a time chosen with uniform random
    206+        // distribution in the range 1/2 to 5/8 of expiration time.
    


    Sjors commented at 10:38 am on May 24, 2024:
    0// 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


    laanwj commented at 5:08 pm on May 24, 2024:

    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.

  116. laanwj commented at 11:33 am on May 24, 2024: member

    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.

  117. Sjors commented at 11:34 am on May 24, 2024: member

    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.

  118. maflcko commented at 11:46 am on May 24, 2024: member
    For testing, something like #30164 could be used
  119. in src/util/pcp.cpp:259 in 9a265c6d75 outdated
    247+                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s: Could not receive response: %s\n", protocol, NetworkErrorString(WSAGetLastError()));
    248+                return std::nullopt; // Network-level error, probably no use retrying.
    249+            }
    250+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "%s: Received response of %d bytes: %s\n", protocol, recvsz, HexStr(Span(response, recvsz)));
    251+
    252+            if (check_packet(Span<uint8_t>(response, recvsz))) {
    


    Sjors commented at 12:10 pm on May 24, 2024:
    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?

    laanwj commented at 4:57 pm on May 24, 2024:
    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

    laanwj commented at 7:07 pm on May 24, 2024:

    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.

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

    Sjors commented at 7:20 am on May 27, 2024:
    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.

    laanwj commented at 7:52 am on May 27, 2024:
    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.
  120. Sjors commented at 12:10 pm on May 24, 2024: member

    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.

  121. Sjors commented at 12:30 pm on May 24, 2024: member
    The first two commits of #26812 would make it possible to test and fuzz how this code interacts with a router.
  122. laanwj force-pushed on May 24, 2024
  123. laanwj commented at 5:13 pm on May 24, 2024: member
    1cbbcba42cf1c050022a73cb02b9d385fa184f2e: squashed all fixups to clean up the list of commits, no other changes
  124. DrahtBot removed the label CI failed on May 24, 2024
  125. laanwj commented at 7:47 am on May 25, 2024: member

    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)

  126. Sjors commented at 8:33 am on May 27, 2024: member

    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?

  127. in src/net.h:151 in bfde83050e outdated
    147@@ -148,7 +148,7 @@ enum
    148     LOCAL_NONE,   // unknown
    149     LOCAL_IF,     // address a local interface listens on
    150     LOCAL_BIND,   // address explicit bound to
    151-    LOCAL_MAPPED, // address reported by UPnP or NAT-PMP
    152+    LOCAL_MAPPED, // address reported by UPnP or PCP
    


    Sjors commented at 8:54 am on May 27, 2024:
    bfde83050ece1f617efdb4098b5efeeb1a08b65e nit: UPnP, PCP or NAT-PMP

    laanwj commented at 10:41 am on May 27, 2024:
    Sure, ok, really i’m just considering NAT-PMP to be PCPv0.
  128. laanwj commented at 9:00 am on May 27, 2024: member

    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.

  129. Sjors commented at 10:14 am on May 27, 2024: member

    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.

  130. laanwj commented at 10:45 am on May 27, 2024: member

    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.

  131. Sjors commented at 10:50 am on May 27, 2024: member

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

  132. laanwj commented at 11:02 am on May 27, 2024: member

    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…

  133. Sjors commented at 11:17 am on May 27, 2024: member
    On macOS I recompiled with PORT_MAPPING_REANNOUNCE_PERIOD set to 5 minutes and IPv4 commented out. At startup it adds three mappings. After a few minutes at renewal it complains with NO_RESOURCES
  134. laanwj commented at 11:21 am on May 27, 2024: member

    After a few minutes at renewal it complains with NO_RESOURCES.

    Does it complain for all three mappings?

  135. Sjors commented at 11:25 am on May 27, 2024: member

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

  136. Sjors commented at 11:30 am on May 27, 2024: member

    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.

  137. laanwj commented at 11:48 am on May 27, 2024: member
    That does look like an issue that would affect renewal, but i’m not sure it would cause this specific issue, if i understand correctly it’d bump the renewal timestamp so far it would live forever. It wouln’t make it complain about the port being used. Unless it’s an old mapping that sticks around. But that wouldn’e explain why it only happens on renewal.
  138. Sjors commented at 12:06 pm on May 27, 2024: member

    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.

  139. laanwj commented at 12:10 pm on May 27, 2024: member

    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.

  140. Sjors commented at 12:29 pm on May 27, 2024: member

    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.

  141. in src/util/pcp.cpp:22 in 97ae5d4eef outdated
    17+
    18+// RFC6886 NAT-PMP and RFC6887 Port Control Protocol (PCP) implementation.
    19+// NAT-PMP and PCP use network byte order (big-endian).
    20+
    21+// NAT-PMP (v0) protocol constants.
    22+//! NAT-PMP uses a fixed server port number (RPC6887 section 1.1).
    


    Sjors commented at 12:38 pm on May 27, 2024:
    RPC -> RFC (in a few other places too)
  142. laanwj commented at 12:40 pm on May 28, 2024: member
    Thanks for investigating in detail. FWIW, i’m not seeing that problem on TurrisOS (openwrt) with miniupnpd 2.3.3. Renewals work fine, both with 20 minute and 5 minute reannounce period. Both for IPv4 and IPv6. So it’s either something that was solved in the meantime, or something different in your networking situation.
  143. Sjors commented at 1:17 pm on May 28, 2024: member

    Both our setups are at miniupnpd 2.3.3; the plugin description incorrectly says 2.3.1: https://github.com/opnsense/plugins/issues/4003#issuecomment-2133421334

    So perhaps there’s a difference in how miniupnpd controls the router / firewall itself, which causes the renewal to fail on my end. This is above my pay grade…

  144. laanwj commented at 1:27 pm on May 28, 2024: member

    i’ve been trying to build the package myself, and there’s miniupnpd-nftables and miniupnpd-iptables, do you know which one you have? i see i have -iptables installed at the moment, it appears.

    Edit: i installed the -nftables variant and rebooted, and it’s giving me NO_RESOURCES even on first rquest, for IPv4 and IPv6! Restored the -iptables variant and it worked again. No idea if it’s the same issue of course. May just be a kernel issue.

  145. Sjors commented at 2:41 pm on May 28, 2024: member

    No idea. OPNsense is built on FreeBSD and I think the plugin just relies on whatever miniupnpd comes with the OS:

     0# pkg info miniupnpd
     1miniupnpd-2.3.3_3,1
     2Name           : miniupnpd
     3Version        : 2.3.3_3,1
     4Installed on   : Thu May  2 10:44:17 2024 CEST
     5Origin         : net/miniupnpd
     6Architecture   : FreeBSD:13:amd64
     7Prefix         : /usr/local
     8Categories     : net
     9Licenses       : BSD3CLAUSE
    10Maintainer     : squat@squat.no
    11WWW            : http://miniupnp.free.fr/
    12Comment        : UPnP IGD implementation which uses pf
    13Options        :
    14	CHECK_PORTINUSE: on
    15	IPV6           : on
    16	LEASEFILE      : off
    17	UPNP_IGDV2     : off
    18	UPNP_STRICT    : off
    19Shared Libs required:
    20	libssl.so.12
    21	libpfctl.so.0
    22	libcrypto.so.12
    23Annotations    :
    24	FreeBSD_version: 1302001
    25	cpe            : cpe:2.3:a:miniupnp_project:miniupnpd:2.3.3:::::freebsd13:x64:3
    26	repo_type      : binary
    27	repository     : OPNsense
    28Flat size      : 160KiB
    29Description    :
    30Mini UPnPd is a lightweight implementation of a UPnP IGD daemon. This is
    31supposed to be run on your gateway machine to allow client systems to redirect
    32ports and punch holes in the firewall.
    

    I installed 2.3.6 from source and got the same NO_RESOURCES error upon IPv6 pinhole renewal. Tried 5fcf0c281fd4e3fa3f32114824c1dee8f78cca03 for good measure. Ditto fail.


    Update: @laanwj figured it out pinned it down: https://github.com/miniupnp/miniupnp/issues/747

  146. bitcoin deleted a comment on May 28, 2024
  147. in src/util/netif.cpp:39 in 357ab7179d outdated
    28+    const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
    29+    if (s < 0) {
    30+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", NetworkErrorString(errno));
    31+        return std::nullopt;
    32+    }
    33+    Sock sock{static_cast<SOCKET>(s)};
    


    vasild commented at 12:54 pm on May 29, 2024:

    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 socket(int domain, int type, int protocol)? …

    Almost. Right now we have the “pointer”

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

    which in the real code points to the function

    0std::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?


    Sjors commented at 4:09 pm on May 29, 2024:
    @vasild I haven’t had a chance to thoroughly review the second commit. Making a separate PR could help.

    laanwj commented at 9:00 am on May 30, 2024:

    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.


    vasild commented at 11:46 am on May 30, 2024:
    Extended CreateSock() in the topmost commit in https://github.com/vasild/bitcoin/commits/extend_CreateSock/. Separate PR or include in this PR?

    Sjors commented at 12:37 pm on May 30, 2024:
    This PR is already quite large, so let’s make a fresh one that this can rebase on if needed.

    laanwj commented at 1:07 pm on May 30, 2024:
    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.

    vasild commented at 1:41 pm on May 30, 2024:
    Opened #30202 “netbase: extend CreateSock() to support creating arbitrary sockets”

    vasild commented at 3:51 pm on May 30, 2024:

    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


    Sjors commented at 4:32 pm on May 30, 2024:
    I got a bit confused about these two new PRs, but I guess they are orthogonal.
  148. DrahtBot commented at 1:10 am on May 30, 2024: contributor

    🚧 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/25552745115

  149. DrahtBot added the label CI failed on May 30, 2024
  150. Sjors commented at 7:41 am on May 30, 2024: member

    I was able to build b4535d48aed26c3b3f61c8839b041d076b02d132 on my BSD 13.2 VM (errors and warnings are gone).

    Opening ports fails, maybe because it’s a VM, but “Address family not supported by protocol family” seems an odd error for that.

    02024-05-23T01:58:47Z [net:error] socket(AF_NETLINK): Address family not supported by protocol family (47)
    12024-05-23T01:58:47Z [net] portmap: Could not determine IPv4 default gateway
    22024-05-23T01:58:47Z [net:error] socket(AF_NETLINK): Address family not supported by protocol family (47)
    32024-05-23T01:58:47Z addcon thread start
    42024-05-23T01:58:47Z [net] portmap: Could not determine IPv6 default gateway
    

    On BSB 14.0 it seems to work fine:

     02024-05-27T17:37:34Z mapport thread start
     12024-05-27T17:37:34Z [net] portmap: gateway [IPv4]: 10.0.2.2
     22024-05-27T17:37:34Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 38333 from gateway 10.0.2.2
     32024-05-27T17:37:34Z Bound to [::]:38333
     42024-05-27T17:37:34Z [net] pcp: Internal address after connect: 10.0.2.15
     52024-05-27T17:37:34Z Bound to 0.0.0.0:38333
     62024-05-27T17:37:35Z [net] pcp: Timeout
     72024-05-27T17:37:35Z [net] pcp: Retrying (1)
     82024-05-27T17:37:36Z [net] pcp: Timeout
     92024-05-27T17:37:36Z [net] pcp: Retrying (2)
    102024-05-27T17:37:37Z [net] pcp: Timeout
    112024-05-27T17:37:37Z [net] pcp: Giving up after 3 tries
    

    (it doesn’t actually get a mapping, but that’s expected because VirtualBox doesn’t come with PCP, afaik)

  151. laanwj commented at 7:45 am on May 30, 2024: member

    Thanks for testing. Sounds like AF_NETLINK is not actually supported on that kernel, even though the necessary stuff is in the headers.

    On BSB 14.0 it seems to work fine:

    So let’s bump the minimum to FreeBSD 14.0 instead?

    (it doesn’t actually get a mapping, but that’s expected because VirtualBox doesn’t come with PCP, afaik)

    It’d likely work if you set VirtualBox’s VM networking to bridged instead of local NAT.

  152. Sjors commented at 12:39 pm on May 30, 2024: member

    With a bridged network it indeed works.

    Also, as suggested by @vasild, doing kldload /boot/kernel/netlink.ko makes things work on FreeBSD 13.2.

  153. DrahtBot removed the label CI failed on May 30, 2024
  154. Self-Hosting-Group commented at 7:10 pm on May 31, 2024: none
    Maybe too late or has been looked at. A PCP/NAT-PMP library https://github.com/libpcp/pcp
  155. laanwj commented at 10:25 am on June 1, 2024: member

    Maybe too late or has been looked at. A PCP/NAT-PMP library https://github.com/libpcp/pcp

    Yes, some revievers have been looking at it as reference/comparison.

    This has been said before but: implementing a self-contained version of the RFCs here is intentional, we don’t want to introduce a dependency on a third-party library. It shoudn’t be needed for a straightforward fixed packet-based protocol. The intent is to have code that integrates with bitcoin core’s frameworks for logging, networking and testing/fuzzing. At some point we want to be confident enough about it to enable it by default.

  156. Self-Hosting-Group commented at 6:50 am on June 11, 2024: none
    @laanwj Thank you for the explanation.
  157. DrahtBot added the label Needs rebase on Jun 12, 2024
  158. laanwj force-pushed on Jun 15, 2024
  159. laanwj commented at 1:32 pm on June 15, 2024: member

    Force push: rebased for small conflict in src/Makefile.am, collected fixup commits, added @vasild as co-author on the netif commit, no other changes.

    Edit: also had to change a util::HasPrefix to fix the build.

  160. laanwj force-pushed on Jun 15, 2024
  161. DrahtBot added the label CI failed on Jun 15, 2024
  162. laanwj force-pushed on Jun 15, 2024
  163. DrahtBot removed the label Needs rebase on Jun 15, 2024
  164. DrahtBot removed the label CI failed on Jun 15, 2024
  165. bitcoin deleted a comment on Jun 15, 2024
  166. in src/util/netif.cpp:22 in a8818568a4 outdated
    15+#ifdef __FreeBSD__
    16+#include <osreldate.h>
    17+#endif
    18+
    19+// Linux and FreeBSD 13.2+
    20+#if defined(__linux__) || __FreeBSD_version >= 1400000
    


    Sjors commented at 12:39 pm on June 17, 2024:
    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.

    laanwj commented at 9:58 am on June 19, 2024:
    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

    Sjors commented at 8:12 am on June 20, 2024:
    Good point, maybe just add a comment about this. FreeBSD 13 won’t be EOL for another two years, so someone else might wonder.

    vasild commented at 11:30 am on June 21, 2024:
    The comment says 13.2+ but the code says 1400000
  167. in doc/build-osx.md:146 in 3fdc97a2bf outdated
    141-
    142-``` bash
    143-brew install libnatpmp
    144-```
    145-
    146-Note: UPnP and NAT-PMP support will be compiled in and disabled by default.
    


    Sjors commented at 2:07 pm on June 17, 2024:

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

    0Note: UPnP support will be compiled in, but disabled by default.
    
  168. in doc/build-osx.md:147 in 3fdc97a2bf outdated
    142-``` bash
    143-brew install libnatpmp
    144-```
    145-
    146-Note: UPnP and NAT-PMP support will be compiled in and disabled by default.
    147-Check out the [further configuration](#further-configuration) section for more information.
    


    Sjors commented at 2:09 pm on June 17, 2024:
    3fdc97a2bf7d1ec774d2ffa00502188158c64724: I don’t know why this was here in the first place, seems fine to drop.
  169. in src/init.cpp:1861 in 57e23f3b4d outdated
    1803@@ -1808,7 +1804,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    1804     LogPrintf("nBestHeight = %d\n", chain_active_height);
    1805     if (node.peerman) node.peerman->SetBestBlock(chain_active_height, std::chrono::seconds{best_block_time});
    1806 
    1807-    // Map ports with UPnP or NAT-PMP.
    


    Sjors commented at 2:15 pm on June 17, 2024:
    57e23f3b4d9cc5bdba421e47def9f3b2335d4cfb: nit, no need to drop this
  170. Sjors approved
  171. Sjors commented at 2:18 pm on June 17, 2024: member

    tACK faacd264417bd7f3f08c5ff497458030b3a54fbc

    Tested on Intel macOS 13.6.7, FreeBSD 13.2, Windows and Ubuntu 24.04.

    Letting Windows run a bit longer to see if its “defender” leaves us alone now.

  172. DrahtBot requested review from vasild on Jun 17, 2024
  173. DrahtBot requested review from theuni on Jun 17, 2024
  174. DrahtBot added the label CI failed on Jun 18, 2024
  175. DrahtBot removed the label CI failed on Jun 18, 2024
  176. in src/util/pcp.cpp:26 in 44316c3900 outdated
    18+// RFC6886 NAT-PMP and RFC6887 Port Control Protocol (PCP) implementation.
    19+// NAT-PMP and PCP use network byte order (big-endian).
    20+
    21+// NAT-PMP (v0) protocol constants.
    22+//! NAT-PMP uses a fixed server port number (RFC6887 section 1.1).
    23+constexpr uint16_t NATPMP_SERVER_PORT = 5351;
    


    sipa commented at 7:07 pm on June 18, 2024:

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

  177. in src/util/pcp.cpp:259 in 44316c3900 outdated
    250+                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s: Could not receive response: %s\n", protocol, NetworkErrorString(WSAGetLastError()));
    251+                return std::nullopt; // Network-level error, probably no use retrying.
    252+            }
    253+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "%s: Received response of %d bytes: %s\n", protocol, recvsz, HexStr(Span(response, recvsz)));
    254+
    255+            if (check_packet(Span<uint8_t>(response, recvsz))) {
    


    sipa commented at 7:09 pm on June 18, 2024:

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


    laanwj commented at 5:26 pm on June 24, 2024:

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

    0../../src/util/pcp.cpp:258:41: warning: narrowing conversion of ‘recvsz’ from ‘int’ to ‘std::size_t’ {aka ‘long unsigned int’} [-Wnarrowing]
    1  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?


    sipa commented at 5:59 pm on June 24, 2024:
    In that case you could use check_packet(Span(response, recvsz)) (the <uint8_t> can be automatically deducted).
  178. in src/util/pcp.cpp:217 in 44316c3900 outdated
    212+}
    213+
    214+//! PCP or NAT-PMP send-receive loop.
    215+static std::optional<std::vector<uint8_t>> PCPSendRecv(Sock &sock, const std::string &protocol, Span<const uint8_t> request, int num_tries,
    216+        std::chrono::milliseconds timeout_per_try,
    217+        std::function<bool(const Span<const uint8_t>)> check_packet)
    


    sipa commented at 7:10 pm on June 18, 2024:

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

  179. in src/util/pcp.cpp:298 in 44316c3900 outdated
    288+    if (sock.Connect((struct sockaddr*)&dest_addr, dest_addrlen) != 0) {
    289+        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "natpmp: Could not connect to gateway: %s\n", NetworkErrorString(WSAGetLastError()));
    290+        return MappingError::NETWORK_ERROR;
    291+    }
    292+
    293+    // Use getsockname to get the address toward the default gateway (the internal address).
    


    sipa commented at 7:22 pm on June 18, 2024:

    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.


    laanwj commented at 10:05 am on June 19, 2024:
    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.
  180. in src/util/pcp.h:54 in 44316c3900 outdated
    49+//! * port: Internal port, and desired external port.
    50+//! * lifetime: Requested lifetime in seconds for mapping. The server may assign as shorter or longer lifetime. A lifetime of 0 deletes the mapping.
    51+//! * num_tries: Number of tries in case of no response.
    52+//!
    53+//! Returns the external_ip:external_port of the mapping if successful, otherwise a MappingError.
    54+std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &gateway, uint16_t port, uint32_t lifetime, int num_tries = 3, std::chrono::milliseconds timeout_per_try = std::chrono::milliseconds(1000));
    


    sipa commented at 7:44 pm on June 18, 2024:

    In commit “net: Add PCP and NATPMP implementation”

    Nit, if you want somewhat more brevity:

    0using namespace std::chrono_literals;
    1
    2... NATPMPRequestPortMap(..., std::chrono::milliseconds timeout_per_try = 1s);
    

    (and below)


    laanwj commented at 10:07 am on June 19, 2024:
    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?

    sipa commented at 3:29 pm on June 19, 2024:
    @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.
  181. in src/util/pcp.cpp:431 in 44316c3900 outdated
    426+    LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "pcp: Internal address after connect: %s\n", internal.ToStringAddr());
    427+
    428+    // Build request packet. Make sure the packet is zeroed so that reserved fields are zero
    429+    // as required by the spec (and not potentially leak data).
    430+    // Make sure there's space for the request header and MAP specific request data.
    431+    std::vector<uint8_t>request(PCP_HDR_SIZE + PCP_MAP_SIZE);
    


    sipa commented at 7:47 pm on June 18, 2024:

    In commit “net: Add PCP and NATPMP implementation”

    Nit: space after type

  182. sipa commented at 7:49 pm on June 18, 2024: member
    Just a quick look so far.
  183. achow101 referenced this in commit a961ad1beb on Jun 20, 2024
  184. in src/util/netif.cpp:39 in faacd26441 outdated
    34+    const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
    35+    if (s < 0) {
    36+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", NetworkErrorString(errno));
    37+        return std::nullopt;
    38+    }
    39+    Sock sock{static_cast<SOCKET>(s)};
    


    vasild commented at 3:09 pm on June 21, 2024:

    Now that #30202 has been merged this can be:

     0     // Create a netlink socket.
     1-    const int s{socket(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
     2-    if (s < 0) {
     3+    auto sock{CreateSock(AF_NETLINK, SOCK_DGRAM, NETLINK_ROUTE)};
     4+    if (!sock) {
     5         LogPrintLevel(BCLog::NET, BCLog::Level::Error, "socket(AF_NETLINK): %s\n", NetworkErrorString(errno));
     6         return std::nullopt;
     7     }
     8-    Sock sock{static_cast<SOCKET>(s)};
     9
    10     // Send request.
    

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

    This would allow mocking of the socket creation.


    laanwj commented at 4:27 pm on June 24, 2024:
    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
  185. in src/util/netif.cpp:49 in faacd26441 outdated
    44+        rtmsg data; ///< Request data, a "route message".
    45+        nlattr dst_hdr; ///< One attribute, conveying the route destination address.
    46+        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.
    47+    } request{};
    48+
    49+    // Whether to use the first 4 or 16 bytes from request.attr_dst_data.
    


    vasild commented at 3:11 pm on June 21, 2024:
    0    // Whether to use the first 4 or 16 bytes from request.dst_data.
    
  186. in src/util/netif.cpp:115 in faacd26441 outdated
    105+        // Found gateway?
    106+        if (rta_gateway != nullptr) {
    107+            if (family == AF_INET) {
    108+                Assume(sizeof(in_addr) == RTA_PAYLOAD(rta_gateway));
    109+                in_addr gw;
    110+                std::memcpy(&gw, RTA_DATA(rta_gateway), sizeof(gw));
    


    vasild commented at 3:25 pm on June 21, 2024:

    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)


    laanwj commented at 5:43 pm on June 24, 2024:
    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.
  187. in src/util/netif.cpp:244 in faacd26441 outdated
    237+
    238+// Dummy implementation.
    239+static std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
    240+{
    241+    (void)family;
    242+    return std::nullopt;
    


    vasild commented at 4:11 pm on June 21, 2024:

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

    0static std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t)
    1{
    2    return std::nullopt;
    
  188. in src/util/netif.cpp:277 in faacd26441 outdated
    272+{
    273+    std::vector<CNetAddr> addresses;
    274+#ifdef WIN32
    275+    char pszHostName[256] = "";
    276+    if (gethostname(pszHostName, sizeof(pszHostName)) != SOCKET_ERROR)
    277+    {
    


    vasild commented at 4:15 pm on June 21, 2024:
    style: { should be on the same line as if or for (also in a few places below).
  189. vasild commented at 4:39 pm on June 21, 2024: contributor
    Posting review midway, up to 237792601fb911cf2e5abebc9226d63f4cd35cec (incl)
  190. DrahtBot requested review from vasild on Jun 21, 2024
  191. laanwj force-pushed on Jun 24, 2024
  192. laanwj commented at 6:11 pm on June 24, 2024: member
    Force push: rebased to get CreateSock() from #30202.
  193. laanwj force-pushed on Jun 24, 2024
  194. laanwj force-pushed on Jun 24, 2024
  195. DrahtBot added the label CI failed on Jun 24, 2024
  196. DrahtBot removed the label CI failed on Jun 24, 2024
  197. bitcoin deleted a comment on Jun 24, 2024
  198. laanwj force-pushed on Jun 26, 2024
  199. in src/util/pcp.h:53 in 23916f2a77 outdated
    48+//! * gateway: Destination address for PCP requests (usually the default gateway).
    49+//! * port: Internal port, and desired external port.
    50+//! * lifetime: Requested lifetime in seconds for mapping. The server may assign as shorter or longer lifetime. A lifetime of 0 deletes the mapping.
    51+//! * num_tries: Number of tries in case of no response.
    52+//!
    53+//! Returns the external_ip:external_port of the mapping if successful, otherwise a MappingError.
    


    vasild commented at 1:20 pm on June 26, 2024:

    style: make more doxygen friendly:

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

    before:

    without_doxy

    after:

    with_doxy

  200. in src/util/pcp.cpp:26 in 23916f2a77 outdated
    21+// RFC6886 NAT-PMP and RFC6887 Port Control Protocol (PCP) implementation.
    22+// NAT-PMP and PCP use network byte order (big-endian).
    23+
    24+// NAT-PMP (v0) protocol constants.
    25+//! NAT-PMP uses a fixed server port number (RFC6887 section 1.1).
    26+constexpr uint16_t NATPMP_SERVER_PORT = 5351;
    


    vasild commented at 1:37 pm on June 26, 2024:

    “RFC6887 section 1.1” should be “RFC6886 section 1.1”?

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

  201. laanwj commented at 2:59 pm on June 26, 2024: member
    Force push was to squash all fixups (no other changes).
  202. in src/util/pcp.cpp:229 in 23916f2a77 outdated
    224+    uint8_t response[PCP_MAX_SIZE];
    225+    bool got_response = false;
    226+    int recvsz = 0;
    227+    for (int ntry = 0; !got_response && ntry < num_tries; ++ntry) {
    228+        if (ntry > 0) {
    229+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "%s: Retrying (%d)\n", protocol, ntry);
    


    vasild commented at 3:10 pm on June 26, 2024:
    nit: maybe worth logging also num_tries, e.g. “retrying 1/5”, “retrying 2/5”, …
  203. in src/util/pcp.cpp:247 in 23916f2a77 outdated
    242+            if (!sock.Wait(deadline - cur_time, Sock::RECV, &occurred)) {
    243+                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "%s: Could not wait on socket: %s\n", protocol, NetworkErrorString(WSAGetLastError()));
    244+                return std::nullopt; // Network-level error, probably no use retrying.
    245+            }
    246+            if (!occurred) {
    247+                LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "%s: Timeout\n", protocol);
    


    vasild commented at 3:11 pm on June 26, 2024:
    nit: maybe worth logging ntry and num_tries, e.g. “timeout 1/5”, “timeout 2/5”, …
  204. in src/util/pcp.h:10 in bc41b2f898 outdated
     5+#ifndef BITCOIN_UTIL_PCP_H
     6+#define BITCOIN_UTIL_PCP_H
     7+
     8+#include <netaddress.h>
     9+
    10+#include <variant>
    


    theuni commented at 3:19 pm on June 26, 2024:

    Missing:

    0#include <cstdint>
    1#include <chrono>
    2#include <array>
    

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


    laanwj commented at 5:29 pm on June 26, 2024:
    Will add, thanks!
  205. in src/util/netif.h:8 in 9a067b5ac1 outdated
    0@@ -0,0 +1,20 @@
    1+// Copyright (c) 2024 The Bitcoin Core developers
    2+// Distributed under the MIT software license, see the accompanying
    3+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
    4+
    5+#ifndef BITCOIN_UTIL_NETIF_H
    6+#define BITCOIN_UTIL_NETIF_H
    7+
    8+#include <netaddress.h>
    


    theuni commented at 3:31 pm on June 26, 2024:

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

     0diff --git a/src/netaddress.h b/src/netaddress.h
     1index 52fecada1c9..75d172ab334 100644
     2--- a/src/netaddress.h
     3+++ b/src/netaddress.h
     4@@ -31,3 +31,3 @@
     5  */
     6-enum Network {
     7+enum Network : int {
     8     /// Addresses from these networks are not publicly routable on the global Internet.
     9diff --git a/src/util/netif.cpp b/src/util/netif.cpp
    10index ad3f93b379a..c4d4c3a31c8 100644
    11--- a/src/util/netif.cpp
    12+++ b/src/util/netif.cpp
    13@@ -9,2 +9,3 @@
    14 #include <logging.h>
    15+#include <netaddress.h>
    16 #include <netbase.h>
    17diff --git a/src/util/netif.h b/src/util/netif.h
    18index 5ff473fd4f2..ab31115f0ff 100644
    19--- a/src/util/netif.h
    20+++ b/src/util/netif.h
    21@@ -7,5 +7,7 @@
    22
    23-#include <netaddress.h>
    24-
    25 #include <optional>
    26+#include <vector>
    27+
    28+enum Network : int;
    29+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:

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

    laanwj commented at 5:33 pm on June 26, 2024:
    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.
  206. in src/util/pcp.cpp:293 in 23916f2a77 outdated
    288+        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "natpmp: Could not create UDP socket: %s\n", NetworkErrorString(WSAGetLastError()));
    289+        return MappingError::NETWORK_ERROR;
    290+    }
    291+
    292+    // Associate UDP socket to gateway.
    293+    if (sock->Connect((struct sockaddr*)&dest_addr, dest_addrlen) != 0) {
    


    vasild commented at 3:35 pm on June 26, 2024:

    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:

    0// Associate UDP socket to gateway. This used to get our local address toward the gateway and
    1// to ensure that we only process packets from the gateway, which is mandated by RFC 6886 3.2.
    
  207. in src/util/pcp.cpp:354 in 23916f2a77 outdated
    349+        [&](const Span<const uint8_t> response) -> bool {
    350+            if (response.size() < NATPMP_MAP_RESPONSE_SIZE) {
    351+                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "natpmp: Response too small\n");
    352+                return false; // Wasn't response to what we expected, try receiving next packet.
    353+            }
    354+            if (response[0] != NATPMP_VERSION || response[1] != (NATPMP_RESPONSE | NATPMP_OP_MAP_TCP)) {
    


    vasild commented at 3:50 pm on June 26, 2024:

    Could use the constants instead of magic numbers:

    0            if (response[NATPMP_HDR_VERSION_OFS] != NATPMP_VERSION || response[NATPMP_HDR_OP_OFS] != (NATPMP_RESPONSE | NATPMP_OP_MAP_TCP)) {
    
  208. in src/util/pcp.cpp:227 in 23916f2a77 outdated
    222+    using namespace std::chrono;
    223+    // UDP is a potentially lossy protocol, so we try to send again a few times.
    224+    uint8_t response[PCP_MAX_SIZE];
    225+    bool got_response = false;
    226+    int recvsz = 0;
    227+    for (int ntry = 0; !got_response && ntry < num_tries; ++ntry) {
    


    vasild commented at 4:21 pm on June 26, 2024:

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


    laanwj commented at 11:18 am on July 3, 2024:
    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.
  209. in src/util/pcp.cpp:408 in 23916f2a77 outdated
    403+        return MappingError::NETWORK_ERROR;
    404+    }
    405+
    406+    // Make sure that we send from requested destination address, anything else will be
    407+    // rejected by a security-conscious router.
    408+    if (sock->Bind((struct sockaddr*)&bind_addr, bind_addrlen) != 0) {
    


    vasild commented at 1:42 pm on June 27, 2024:

    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.


    laanwj commented at 12:16 pm on July 3, 2024:

    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)


    vasild commented at 11:24 am on July 17, 2024:

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

  210. in src/util/pcp.cpp:409 in 23916f2a77 outdated
    404+    }
    405+
    406+    // Make sure that we send from requested destination address, anything else will be
    407+    // rejected by a security-conscious router.
    408+    if (sock->Bind((struct sockaddr*)&bind_addr, bind_addrlen) != 0) {
    409+        LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "pcp: Could not bind to address: %s\n", NetworkErrorString(WSAGetLastError()));
    


    vasild commented at 2:38 pm on June 27, 2024:
    Maybe worth logging bind.ToStringAddr() here and gateway.ToStringAddr() below if Connect() fails.
  211. in src/util/pcp.cpp:447 in 23916f2a77 outdated
    442+
    443+    ofs += PCP_HDR_SIZE;
    444+
    445+    // Fill in MAP request packet, See RFC6887 Figure 9.
    446+    // Randomize mapping nonce (this is repeated in the response, to be able to
    447+    // correlate requests and responses, and used to authenticate changes to the mapping).
    


    vasild commented at 4:29 pm on June 27, 2024:

    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”.


    laanwj commented at 12:10 pm on July 3, 2024:
    Yes, i initially had the wrong idea of what it did, should update the comment.

    Sjors commented at 10:11 am on July 8, 2024:

    some lame form of authentication

    https://github.com/miniupnp/miniupnp/issues/748 (I still need to test the fix PR)

  212. in src/util/pcp.cpp:452 in 23916f2a77 outdated
    447+    // correlate requests and responses, and used to authenticate changes to the mapping).
    448+    std::memcpy(request.data() + ofs + PCP_MAP_NONCE_OFS, nonce.data(), PCP_MAP_NONCE_SIZE);
    449+    request[ofs + PCP_MAP_PROTOCOL_OFS] = PCP_PROTOCOL_TCP;
    450+    WriteBE16(request.data() + ofs + PCP_MAP_INTERNAL_PORT_OFS, port);
    451+    WriteBE16(request.data() + ofs + PCP_MAP_EXTERNAL_PORT_OFS, port);
    452+    if (!PCPWrapAddress(Span(request).subspan(ofs + PCP_MAP_EXTERNAL_IP_OFS, ADDR_IPV6_SIZE), bind)) return MappingError::NETWORK_ERROR;
    


    vasild commented at 10:29 am on June 28, 2024:

    https://www.rfc-editor.org/rfc/rfc6887#section-11.1

    Suggested External IP Address: Suggested external IPv4 or IPv6 address. This is useful for refreshing a mapping, especially after the PCP server loses state. If the PCP client does not know the external address, or does not have a preference, it MUST use the address-family-specific all-zeros address

    I think that we should not use bind for this. bind is a local address that we bind(2) to. Can’t be the external address. I guess all-zeros is good enough.


    laanwj commented at 11:23 am on July 3, 2024:
    This is intentional. For IPv4 we provide the all-zeroes address, because the router will use its own (NAT), for IPv6 we want the external address to be the same as the local address (NAT is not a thing, we want pinholing).

    vasild commented at 12:17 pm on July 17, 2024:
    Would all-zeros work as well for IPv6 / pinholing? If yes, then this can probably be simplified to always pass all-zeros.
  213. in src/util/pcp.cpp:462 in 23916f2a77 outdated
    457+    // Receive loop.
    458+    bool is_natpmp = false;
    459+    auto recv_res = PCPSendRecv(*sock, "pcp", request, num_tries, timeout_per_try,
    460+        [&](const Span<const uint8_t> response) -> bool {
    461+            // Unsupported version according to RFC6887 appendix A and RFC6886 section 3.5, can fall back to NAT-PMP.
    462+            if (response.size() == NATPMP_RESPONSE_HDR_SIZE && response[PCP_HDR_VERSION_OFS] == NATPMP_VERSION && response[PCP_RESPONSE_HDR_RESULT_OFS] == NATPMP_RESULT_UNSUPP_VERSION) {
    


    vasild commented at 10:42 am on June 28, 2024:
    According to https://www.rfc-editor.org/rfc/rfc6886#section-3.5 the result code is 2 bytes (which happen to be [2] and [3] in our array). Here we check the second byte response[3] == 1. Even that all result codes are less than 256, I think it is warranted to check that the preceding byte is 0. Either by ReadBE16() == 1 or by response[2] == 0 && response[3] == 1.
  214. in src/util/pcp.cpp:483 in 23916f2a77 outdated
    478+                return false; // Wasn't response to what we expected, try receiving next packet.
    479+            }
    480+            uint8_t protocol = response[PCP_HDR_SIZE + 12];
    481+            uint16_t internal_port = ReadBE16(response.data() + PCP_HDR_SIZE + 16);
    482+            if (protocol != PCP_PROTOCOL_TCP || internal_port != port) {
    483+                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "pcp: Response protocol or port doesn't match request\n");
    


    vasild commented at 12:00 pm on June 28, 2024:
    Here we check the nonce, protocol and port. https://www.rfc-editor.org/rfc/rfc6887#section-11.4 says to check “internal IP address (the destination IP address of the PCP response…” as well.

    laanwj commented at 11:25 am on July 3, 2024:
    As we receive the UDP packet at all, i think that should be correct. But sure…
  215. in src/util/pcp.cpp:481 in 23916f2a77 outdated
    476+            if (response.subspan(PCP_HDR_SIZE + PCP_MAP_NONCE_OFS, PCP_MAP_NONCE_SIZE) != Span(nonce)) {
    477+                LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "pcp: Mapping nonce mismatch\n");
    478+                return false; // Wasn't response to what we expected, try receiving next packet.
    479+            }
    480+            uint8_t protocol = response[PCP_HDR_SIZE + 12];
    481+            uint16_t internal_port = ReadBE16(response.data() + PCP_HDR_SIZE + 16);
    


    vasild commented at 12:13 pm on June 28, 2024:

    Replace magic numbers with the constants:

    0            uint8_t protocol = response[PCP_HDR_SIZE + PCP_MAP_PROTOCOL_OFS];
    1            uint16_t internal_port = ReadBE16(response.data() + PCP_HDR_SIZE + PCP_MAP_INTERNAL_PORT_OFS);
    
  216. in src/mapport.cpp:66 in 23916f2a77 outdated
    84+    auto handle_mapping = [&](std::variant<MappingResult, MappingError> &res) -> void {
    85+        if (MappingResult* mapping = std::get_if<MappingResult>(&res)) {
    86+            LogPrintLevel(BCLog::NET, BCLog::Level::Info, "portmap: Added mapping %s\n", mapping->ToString());
    87+            AddLocal(mapping->external, LOCAL_MAPPED);
    88+            ret = true;
    89+            actual_lifetime = std::min(actual_lifetime, mapping->lifetime);
    


    vasild commented at 1:32 pm on June 28, 2024:

    Why choose the minimum? To me this looks like it should be:

    0actual_lifetime = mapping->lifetime;
    

    Is it because there is just one actual_lifetime for all mappings? What about storing each (successful) map? Then we would have individual actual lifetimes and the assigned port, to be used for renewals.


    laanwj commented at 11:27 am on July 3, 2024:
    Again, i intentionally use a simple imlementation here. i don’t want to complicate the code too much. As i understand, this covers the practical cases. It chooses the minimum because we don’t want holes in coverage while renewing the mapping more than necessary doesn’t hurt.
  217. in src/mapport.cpp:102 in 23916f2a77 outdated
    126-    return false;
    127-}
    128+        // IPv6
    129+        std::optional<CNetAddr> gateway6 = QueryDefaultGateway(NET_IPV6);
    130+        if (!gateway6) {
    131+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "portmap: Could not determine IPv6 default gateway\n");
    


    vasild commented at 1:38 pm on June 28, 2024:
    Should the attempts to do IPv6 only be done if g_reachable_nets.Contains(NET_IPV6) is true? Same for IPv4.

    laanwj commented at 11:53 am on July 3, 2024:
    Yes, that makes sense, in the case of onlynet=ipv6 we wouldn’t want to advertise a port on ipv4 and vice versa.
  218. in src/mapport.cpp:90 in 23916f2a77 outdated
    109+            LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "portmap: gateway [IPv4]: %s\n", gateway4->ToStringAddr());
    110+
    111+            // Open a port mapping on whatever local address we have toward the gateway.
    112+            struct in_addr inaddr_any;
    113+            inaddr_any.s_addr = htonl(INADDR_ANY);
    114+            auto res = PCPRequestPortMap(pcp_nonce, *gateway4, CNetAddr(inaddr_any), private_port, requested_lifetime);
    


    vasild commented at 1:45 pm on June 28, 2024:

    This would use the same port for renewals as well, but https://www.rfc-editor.org/rfc/rfc6886#section-3.3 says:

    The renewal packet should look exactly the same as a request packet, except that the client SHOULD set the Suggested External Port to what the NAT gateway previously mapped, not what the client originally suggested.


    laanwj commented at 11:56 am on July 3, 2024:
    i’ve thought about this, but due to how bitcoin handles non-standard ports, we really want the configured external port, not another one, i think it makes sense to keep trying to get it, even if the router assigns us a different one once.

    Sjors commented at 7:57 am on July 8, 2024:
    Indeed. Maybe just add a comment that this is intentional.

    vasild commented at 4:19 pm on July 16, 2024:

    Ok, I am fine with just adding a comment and leave the code as is. This is probably too edge case to bother complicating the code with it, but just mentioning why I think it is better to keep using the initially assigned port even if it is different than the one we requested:

    • We request port 8333, but for whatever reason the router assigns 15000 with an external address let’s say 1.2.3.4.
    • We start using and advertise 1.2.3.4:15000 to other peers, it propagates into nodes’ addrmans. The ones that actually connect to us will have it as good in their tried tables.
    • Eventually we renew but try port 8333 as in the initial PCP request. This time it succeeds so we get 8333, or it is still unavailable, but we get another random port e.g. 16000. We start advertising 1.2.3.4:8333 or 1.2.3.4:16000 and the good old 1.2.3.4:15000 is now junk in nodes’ addrmans. This can repeat over and over with different random ports.

    vasild commented at 9:37 am on July 17, 2024:

    due to how bitcoin handles non-standard ports, we really want the configured external port

    Bitcoin Core had a strong preference to connect to peers that listen on port 8333. Is this what you mean? That was removed at some point.


    jonatack commented at 2:10 pm on August 8, 2024:

    Bitcoin Core had a strong preference to connect to peers that listen on port 8333. Is this what you mean? That was removed at some point.

    Yes, that preference for clearnet connections was removed in #23542.

  219. in src/mapport.cpp:109 in 23916f2a77 outdated
    157-            } else {
    158-                LogPrintf("natpmp: Port mapping failed.\n");
    159+            // Try to open pinholes for all routable local IPv6 addresses.
    160+            for (const auto &addr: GetLocalAddresses()) {
    161+                if (!addr.IsRoutable() || !addr.IsIPv6()) continue;
    162+                auto res = PCPRequestPortMap(pcp_nonce, *gateway6, addr, private_port, requested_lifetime);
    


    vasild commented at 2:54 pm on June 28, 2024:
    Why make mappings for all local IPv6 addresses? Given that this is the same gateway, I think it is going to result in multiple mappings on the same external IPv6 address but on different ports.

    vasild commented at 3:52 pm on June 28, 2024:
    Also, it just occurred to me, GetLocalAddresses() operates regardless of -bind= options. If the machine has two IPv6 addresses but -bind= has been used and only one of them specified, then we shouldn’t do mapping for the other one (where nobody is listening).

    laanwj commented at 11:58 am on July 3, 2024:

    Why make mappings for all local IPv6 addresses?

    It’s the sensible choice IMO. Some routers will assign a whole bunch of routable IPv6 addresses but only allow pinholing for some (eg not for temporary privacy addresses), without a way to distinguish them. It’s doesn’t hurt to try.

    I think it is going to result in multiple mappings on the same external IPv6 address but on different ports.

    i don’t think so? We only request one mapping per IPv6 address at most.

    Also, it just occurred to me, GetLocalAddresses() operates regardless of -bind= option

    Yes. But mind that -natpmp and -pnp are set to 0 if -bind is specified. In that case we assume the user has a more complex configuration and will just do their own mapping. Whoops this is not the case. Only for -proxy and -listen=0. Yes, makes sense to take explicit binds into account if specified.


    Sjors commented at 7:54 am on July 8, 2024:

    I think it is going to result in multiple mappings on the same external IPv6 address but on different ports.

    i don’t think so? We only request one mapping per IPv6 address at most.

    Afaik PCP always uses a pinhole, which does not involve assigning a (different) port.

    We do end up with our node being announced multiple times with different IP addresses, which could cause some other node to connect to us twice. That can already happen when you have IPv4 and IPv6 (and Tor and I2P) enabled.

    If we’re really worried about this, it might make sense to try each IPv6 address sequentially and stop when one is acknowledged. I think that can wait for a followup.


    vasild commented at 1:27 pm on July 17, 2024:
    Ok, I was having NAT in mind when I wrote “is going to result in multiple mappings on the same external IPv6 address”. Sorry for the noise.

    vasild commented at 1:15 pm on July 19, 2024:

    In addition to the -bind thing there is also a complication with the port: if -bind=10.0.0.1:20000 -bind=[5566::1]:30000 has been used, then GetListenPort(), currently used by mapport.cpp, is not the right thing to do because it would return 20000.

    To resolve both, we could get the listening addresses + the correct ports from:

    1. AddLocal() / mapLocalHost, is called after bind (good), but also from other places and after the bind it is called only if fDiscover is true (bad).
    2. CConnman::vhListenSocket, has sockets that used BF_DONT_ADVERTISE during bind. Those should be omitted.
    3. -bind arguments, could get it from init.cpp / connOptions. Need to duplicate some of the logic of CConnman::InitBinds().
    4. introduce a new global list of addr:port, similar but not identical to mapLocalHost, seems imperfect because the list is almost the same as mapLocalHost, ideally those addresses should be in one place with proper flags to distinguish them (or maybe not?).

    Here is a crude implementation of 4:

      0diff --git a/src/mapport.cpp b/src/mapport.cpp
      1index a1d584dae9..56d21e792b 100644
      2--- a/src/mapport.cpp
      3+++ b/src/mapport.cpp
      4@@ -48,13 +48,13 @@ static bool ProcessPCP()
      5     // The same nonce is used for all mappings, this is allowed by the spec, and simplifies keeping track of them.
      6     PCPMappingNonce pcp_nonce;
      7     GetRandBytes(pcp_nonce);
      8 
      9     bool ret = false;
     10     bool no_resources = false;
     11-    const uint16_t private_port = GetListenPort();
     12+    const uint16_t private_port = GetListenPort(); // XXX could be the wrong port if -bind is used
     13     // Multiply the reannounce period by two, as we'll try to renew approximately halfway.
     14     const uint32_t requested_lifetime = std::chrono::seconds(PORT_MAPPING_REANNOUNCE_PERIOD * 2).count();
     15     uint32_t actual_lifetime = 0;
     16     std::chrono::milliseconds sleep_time;
     17 
     18     // Local functor to handle result from PCP/NATPMP mapping.
     19@@ -101,14 +101,18 @@ static bool ProcessPCP()
     20         if (!gateway6) {
     21             LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "portmap: Could not determine IPv6 default gateway\n");
     22         } else {
     23             LogPrintLevel(BCLog::NET, BCLog::Level::Debug, "portmap: gateway [IPv6]: %s\n", gateway6->ToStringAddr());
     24 
     25             // Try to open pinholes for all routable local IPv6 addresses.
     26-            for (const auto &addr: GetLocalAddresses()) {
     27-                if (!addr.IsRoutable() || !addr.IsIPv6()) continue;
     28+            const auto addresses =
     29+                WITH_LOCK(g_addresses_for_port_map.mutex, return g_addresses_for_port_map.addresses);
     30+            for (const auto& addr : addresses) {
     31+                if (!addr.IsIPv6()) {
     32+                    continue;
     33+                }
     34                 auto res = PCPRequestPortMap(pcp_nonce, *gateway6, addr, private_port, requested_lifetime);
     35                 handle_mapping(res);
     36             }
     37         }
     38 
     39         // Log message if we got NO_RESOURCES.
     40diff --git a/src/net.cpp b/src/net.cpp
     41index 7c6babc389..b621e76696 100644
     42--- a/src/net.cpp
     43+++ b/src/net.cpp
     44@@ -112,12 +112,15 @@ static const uint64_t RANDOMIZER_ID_ADDRCACHE = 0x1cf2e4ddd306dda9ULL; // SHA256
     45 // Global state variables
     46 //
     47 bool fDiscover = true;
     48 bool fListen = true;
     49 GlobalMutex g_maplocalhost_mutex;
     50 std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(g_maplocalhost_mutex);
     51+
     52+AddressesForPortMap g_addresses_for_port_map;
     53+
     54 std::string strSubVersion;
     55 
     56 size_t CSerializedNetMsg::GetMemoryUsage() const noexcept
     57 {
     58     // Don't count the dynamic memory used for the m_type string, by assuming it fits in the
     59     // "small string" optimization area (which stores data inside the object itself, up to some
     60@@ -3151,14 +3154,17 @@ bool CConnman::Bind(const CService& addr_, unsigned int flags, NetPermissionFlag
     61         if ((flags & BF_REPORT_ERROR) && m_client_interface) {
     62             m_client_interface->ThreadSafeMessageBox(strError, "", CClientUIInterface::MSG_ERROR);
     63         }
     64         return false;
     65     }
     66 
     67-    if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::NoBan)) {
     68-        AddLocal(addr, LOCAL_BIND);
     69+    if (addr.IsRoutable() && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, NetPermissionFlags::NoBan)) {
     70+        if (fDiscover) {
     71+            AddLocal(addr, LOCAL_BIND);
     72+        }
     73+        WITH_LOCK(g_addresses_for_port_map.mutex, g_addresses_for_port_map.addresses.push_back(addr));
     74     }
     75 
     76     return true;
     77 }
     78 
     79 bool CConnman::InitBinds(const Options& options)
     80diff --git a/src/net.h b/src/net.h
     81index 11cb01a95d..ae08603e64 100644
     82--- a/src/net.h
     83+++ b/src/net.h