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

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611838998)
Well, i guess it wouldn't hurt checking (or adding an `Assume`), but i don't think the routing table is supposed to contain error entries.

🤔 glozow reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2074201852)
ACK 63cfa4ce88

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611833441)
Is there a reason for `struct` (seems like a C thing?)
```suggestion
SubPackageState m_subpackage;
```

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611842178)
Seems this will need to be deleted immediately in the next PR?

💬 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_r1611846940)
> Any reason why `conflicting_block_hash` is passed by reference but `replacement_tx` isn't?

`CTransactionRef` is already a pointer https://github.com/bitcoin/bitcoin/blob/e163d864d380956a4c0f89a4d80a76f5aefc9a08/src/primitives/transaction.h#L423-L424

🚀 ryanofsky merged a pull request: "rpc: avoid copying into UniValue"
(https://github.com/bitcoin/bitcoin/pull/30115)

💬 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_r1611850253)
Same here. I'll take them out

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1611850452)
old habits!

💬 theuni commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1611851597)
Why is this needed?

💬 ryanofsky commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2127356721)
Went ahead and merged this. It seems the increased peak memory usage is not a major concern, and other metrics like number of allocations and run time do seem improved.

Marco seems right that naively calling `shrink_to_fit might` not help much, or could be basically equivalent to copying, and there might be better ways to decrease memory usage, such as with a `write_and_destroy` method.

(Note: I also edited PR description to remove @ from <span>@</span>willcl-ark, so Will doesn't receive g
...