🤔 Sjors reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2071999619)
Took a cursory look at the linux / bsd approach.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610438227)
82b0bff51fedff2ce5e47cae1c75b9172766d8ef: the `SysErrorString` documentation says you should call `NetworkErrorString`, though that will in turn just call `SysErrorString`, since this is never called under `WIN32`.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610476818)
82b0bff51fedff2ce5e47cae1c75b9172766d8ef: any idea what this does?

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610461378)
82b0bff51fedff2ce5e47cae1c75b9172766d8ef Why only on linux? It seems to exist on FreeBSD too: https://man.freebsd.org/cgi/man.cgi?netlink(4)

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610491684)
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 on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610511267)
I see this was added here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1603876259

💬 theuni commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2125552084)
> I think it might actually be unused at this point, but also have a vauge memory of a Qt related failure, if it's missing.. Have pushed up a change to have it set properly for now.

Thanks, can confirm the correct one is picked up by our configure now.

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2125570369)
**NOTE**, this PR isn't ready to merge; at the very least it has some debug prints that need to be removed.

CI was failing on some platforms (Windows and macOS), in `test/functional/mempool_limit.py: test_mid_package_eviction()`. I think this test is fragile; this very-unrelated PR shouldn't be able to cause it to fail IMO. The problem turned out to be the following:

This test calls the test utility function `fill_mempool()`. This function creates independent physically-large (around 65kb)
...

👍 theuni approved a pull request: "depends: Fetch miniupnpc sources from an alternative website"
(https://github.com/bitcoin/bitcoin/pull/30151#pullrequestreview-2072167896)
Agree with @laanwj and @achow101.

This is unfortunate, but seems like the best solution for now. The checksum should point out any hijinks.

utACK 21b8a14d37c19ce292d5529597e0d52338db48a9

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610537095)
The scope ID is extremely important for the default gateway, as it tends to be a scope-local address.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610540459)
> 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?

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610543822)
Right, will update this (though yeah for POSIX operating systems there's no difference).

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610546508)
Probably worth doing now to make it more clear over time. Let me know what you think.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610547164)
For the FreeBSD versus Linux differences see the comments in the standalone tool here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1604635967
The netlink calls behave differently on Linux and FreeBSD, we don't know why this is.

💬 furszy commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610554624)
> > I'm not sure we should rely on the `pong` alone because that might change over time?
>
> If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.

I don't think thats applicable here. We cannot freely change the initial negotiation phase (the version-verack window) without a BIP for the p2p protocol change. At the protocol level, only features negotiation messages are allowed in this phase. An
...

💬 sdaftuar commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610556549)
Seems more readable to me, thanks.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610559719)
> The rta_gateway check is only after going over all the attributes, right?

Oh wait, I misread the indentation, yes.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610568498)
> 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/

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1610572476)
> > > I'm not sure we should rely on the `pong` alone because that might change over time?
> >
> >
> > If it changes (for example the ping is sent before the verack), the test will already start to fail intermittently and will need to be adjusted anyway.
>
> I don't think thats applicable here. We cannot freely change the initial negotiation phase (the version-verack window) without a BIP for the p2p protocol change. At the protocol level, only features negotiation messages are allowed i
...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610572586)
Ah, some of these comments are worth preserving until we know more or can point to clear documentation elsewhere. I compressed them a bit:

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

#ifdef __FreeBSD__
// Linux IPv4 / IPv6 this must be absent, other
...