💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611597515)
Could make this slightly more readable with comments and two nullptr:

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

🤔 Sjors reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2073809550)
Finished macOS review of QueryDefaultGatewayImpl.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611637097)
I guess you can't just do `for (const struct rt_msghdr* rt : buf)`

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611600021)
A useful hint to the reader what's going on here, and where it's documented:

```cpp
// The size of the available data can be determined by calling sysctl() with
// the NULL argument for oldp. See sysctl(3).
```

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611597943)
```cpp
if (sysctl(/*name=*/mib, /*namelen=*/sizeof(mib) / sizeof(int), /*oldp=*/buf.data(), /*oldlenp=*/&l, /*newp=*/nullptr, /*newlen=*/0) < 0) {
```

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611661588)
Do we want to check `rt->rtm_errno` first?

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

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611729605)
```cpp
// Skip sockaddr entries for bit flags we're not interested in,
// move cursor.
```

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611691990)
```cpp
// We only read from this address if a rtm_addrs bit flag is set.
```

💬 glozow commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#issuecomment-2127184039)
Added constant for https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1610126595
Also added a test that wallet does not signal v3 as it uses the default `CURRENT_VERSION` (this is correct as it wouldn't be safe to make transactions v3 before any of the network has adopted this change). I believe the `nVersion` can be changed via `CCoinControl::m_version`, though I'm not sure if that is accessible so I didn't write a test for it.

Rebased on top of #29873 + master (conflict with #29086)
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611755766)
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. Leaving out `ROUNDUP` works fine btw. Might just delete it.

🚀 achow101 merged a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118)

💬 instagibbs commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2127225225)
suggested changes were done, reviewed via `git range-diff master 7c3fb97 ef8de26`

💬 instagibbs commented on pull request "fuzz: More accurate coverage reports":
(https://github.com/bitcoin/bitcoin/pull/30156#issuecomment-2127236772)
concept ACK

💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1611774155)
> As CPFP carve out does not explicitly require a second child to actually
exist, it has the effect of granting a free +10KvB descendant size limit
when a single child is enough to bust the descendant limit.

That sounds like an unintentional behavior. Should that be addressed with a bug fix, or can we just get rid of the carve-out altogether in a world where we have TRUC transactions?

🤔 murchandamus reviewed a pull request: "policy: restrict all TRUC (v3) transactions to 10kvB"
(https://github.com/bitcoin/bitcoin/pull/29873#pullrequestreview-2074103415)
crACK 154b2b2296edccb5ed24e829798dacb6195edc11

💬 glozow commented on pull request "contrib: Renew Windows code signing certificate":
(https://github.com/bitcoin/bitcoin/pull/30149#issuecomment-2127243747)
Backport for 26.x in #29899

💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2127254156)
> Also, please split the interfaces changes into a different commit than the GUI changes.

did that. cleaner and atomic. thanks.

🤔 ismaelsadeeq reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2074136766)
re-ACK 63cfa4ce88cac26abcacf3b243916e722ab17d75

👍 theStack approved a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2074156905)
re-ACK 63cfa4ce88cac26abcacf3b243916e722ab17d75

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1611811645)
The first commit gets rid of it for TRUC, and it'll go away with cluster mempool.

I don't really think a fix in the interim is worth it; it's slightly involved to pull all the ancestors and check that their descendant count exceeds 1. I'd rather focus on getting rid of it tbh.