👍 ryanofsky approved a pull request: "rpc: avoid copying into UniValue"
(https://github.com/bitcoin/bitcoin/pull/30115#pullrequestreview-2072120285)
Code review ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f. No changes since last review other than rebase. I agree benchmarks showing increased peak memory usage and RSS are surprising, but number of allocations is down as expected, and runtime is also decreased.

The fact that "peak allocations" appear to be larger after the change seems interesting. Before the change, the largest allocations are 38.4MB, 26.7MB, and 8.4MB, but after the change they are 39.4MB, 33.7MB, and 10.8MB. I'm not sure
...

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