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

💬 glozow commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1611813658)
Also, note that in a non-TRUC 1p1c scenario, the parent's increased descendant size limit is mooted by the ancestor size limit (of the child).

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611829012)
i don't think so, the pointer needs to advance by the size of the specific message, which is part of that message.

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611830313)
@josibake I looked around a bit and there might be something here. `GetHash()` is called before this point
- when we read the block from disk https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/node/blockstorage.cpp#L1092
- when `ActivateBestChain` is called https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/validation.cpp#L3354

There might be some gain in caching GetHash() but I think that has to be addressed on its own. I
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611831862)
i'm not sure it makes sense to duplicate documentation that's already in the manual pages. Like if we're doing this here, why not for every system call we make. Do parameter names like `*newp=` `oldp=` even elucidate much?

Agree re using `nullptr` where possible.

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1611833798)
The `TxStateBlockConflicted` requires both that's why I added the both of them

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611834614)
Yes, it's a minor optimization, we could always construct a CNetAddr if we wanted, but if we know we're not going to use it anyway might as well skip it.