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

💬 theuni commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2127371384)
@ryanofsky Thanks.

I'm going to continue working on the more complicated copies in follow-up PRs. I'll play around with your `Copy`/`CopyFrom` idea while I'm at it. If nothing else, that would be a good way to help me track down the remaining cases.

💬 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_r1611867224)
> Would also be nice if we prevented these objects from be constructed with `nullptrs`. I'm not sure if we have a convention in our codebase around this or other examples to point to, but would be worth looking into.

It looks like I may be able to do this by deleting the constructor
```
// Deleted constructor
ReplacedReason(std::nullptr_t) = delete;
```
I'll test it

💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10kvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1611867777)
Oh right, I missed that. That mitigates my main concern, and given that this hack is going away eventually, I agree on the lack of urgency.

💬 achow101 commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#issuecomment-2127373603)
There seems to be an intermittent failure with `feature_discover.py`. 2 of 4 functional test suite runs that I just did failed there with:

```
175/307 - feature_discover.py failed, Duration: 1 s

stdout:
2024-05-23T15:01:57.868000Z TestFramework (INFO): PRNG seed is: 6285884694359884871
2024-05-23T15:01:57.869000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20240523_110117/feature_discover_78
2024-05-23T15:01:58.127000Z TestFramework (INFO): Restart node with
...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611882820)
> Also they're variable-length

That makes sense. Whereas `rt_msghdr` is fixed length I guess, so you're able to use `rt + 1` above.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611884887)
I'm not sure, but there reason I wrote this comment is because initially I thought: yikes, what if this is out of bound?

💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1611885584)
Remnant of a long-lost pre-C++20 past. Gone.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1611888217)
Mmm, I thought perhaps it would return a single error entry to indicate failure.