💬 BenWestgate commented on pull request "contrib: verify-binaries accept full arch-platform specifier":
(https://github.com/bitcoin/bitcoin/pull/28418#issuecomment-2125438436)
Concept ACK

Have not tested this branch but based on reading the diff it Closes #30145.

Approach:
The script doesn't need to know what part of the string is `arch`, `os` or `platform` to match the file.
Considering this, a much simpler parser produces the same functionality:
https://github.com/bitcoin/bitcoin/blob/9f6ccfced950cc4e334163af911ffc620bd5e216/contrib/verify-binaries/verify.py#L102-L111
Is there a reason to capture these in separate variables that I am missing?

Comparing
...

💬 sdaftuar commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2125445040)
ACK 154b2b2296edccb5ed24e829798dacb6195edc11

💬 maflcko commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1610472962)
```suggestion
LogInfo("Flushing intermediate state of replay\n");
```

nit: For new code it would be better to use the non-deprecated non-confusing alias `LogInfo` over `LogPrintf`.

💬 sdaftuar commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1610484226)
Do you think it might make sense to explicitly group these variables into a single struct that makes it clear we have one grouping of per-subpackage state? All these variables that are at the same scope level as other things that don't get reset seems maybe a bit messy...

💬 sdaftuar commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2125532662)
utACK a84b57462cd3dfcd059783696aacd796ef1793d4

Left one nit/question for you, but looks good to me regardless.

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