💬 instagibbs commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1610309923)
sgtm

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1610320133)
Added a version check for at least 13.2.

🤔 theStack reviewed a pull request: "rpc: introduce getversion RPC"
(https://github.com/bitcoin/bitcoin/pull/30112#pullrequestreview-2071898266)
Concept ACK

💬 theStack commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#discussion_r1610377253)
I think the cleanest approach to access the version info from a functional test is via `test/config.ini`, which is generated by the build system based on substitutions in `test/config.ini.in`. The values are available as (nested) dictionary via `self.config`. Simple example:

```diff
diff --git a/test/config.ini.in b/test/config.ini.in
index 291599da45..49bf203cf8 100644
--- a/test/config.ini.in
+++ b/test/config.ini.in
@@ -8,6 +8,9 @@
[environment]
PACKAGE_NAME=@PACKAGE_NAME@
PACKA
...

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#issuecomment-2125375660)
> There are also similar changes in #28418 but I didn't check this PR yet to see if they are the same.

This PR includes a **test.py** for the new functionality. My diff is smaller: Only 3 adds to `verify.py`
Either could close my linked issue.

I like my parser better and #28418 's error messages. I will borrow his strings and squash this.

Also my bad for almost duplicating someone else's PR. I did search for "verify.py" but didn't find any issues. I'll give #28418 a review.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1610397733)
After my fix I found another inconsistency, at block [709,640](https://mempool.space/block/0000000000000000000a3a458944554e24d1ef8d3ef8707067b01dcaf71526cd?page=39) and dust limit 992 your index finds 3 while Bitcoin Core finds 2.

I manually looked up the eligible transactions:
1. https://mempool.space/tx/350f06dff749f910b0ad8d918496850c676c22854e24a046304e73bdfe399a02
2. https://mempool.space/tx/0fdda311049e8a1450ef08bab4076c0e00f8e8334c861ed441f24980b8ac6a90
3. https://mempool.space/tx/7
...

💬 achow101 commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2125431251)
ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f

Checked that only `std::move`s were added, and letting the compiler warn for use after move, which there does not appear to be any.

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