💬 instagibbs commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2130129883)
> Ancestor-set based presplitting inside the search is replaced with using the best ancestor set as one of the LIMO set choices.

Can this be elaborated a bit? I've taken a few minutes looking at the commits and it wasn't immediately clear how this all maps to existing public discussions and the comments:

```
// This is an implementation of the (single) LIMO algorithm:
// https://delvingbitcoin.org/t/limo-combining-the-best-parts-of-linearization-search-and-merging/825

...

💬 LarryRuane commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-2130140023)
CI passed, thanks @theStack. I just force-pushed swapping of the two commits, the test commit has to be first for each commit to pass CI.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1613878905)
removed references to v3, and added a note that non-standard relay args should be removed once they're made standard

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1613878961)
made an attempt at a grepp-able cautionary note

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1613879170)
right this is just a logging arg; probably should clean that up to just take `Wtxid` in future work. re-arranged and passed in child workspace now

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1613879250)
fixed

💬 BenWestgate commented on pull request "doc: Change GiB to GB in release-process.md":
(https://github.com/bitcoin/bitcoin/pull/30171#issuecomment-2130157516)
> I don't think that's universally true. I believe Apple generally uses GB (meaning 109), Microsoft generally uses GB (meaning 230), and open-source systems use a mix (sometimes using GiB to mean 230).

Good to know. Should the value we display depend on platform then? At the very least I think we should be consistent within the repository or within a file. Currently we do math that adds and subtracts GiB from GB in the GUI welcome screen which will cause issues. https://github.com/bitcoin-cor
...

🤔 murchandamus reviewed a pull request: "Fix waste calculation in SelectionResult"
(https://github.com/bitcoin/bitcoin/pull/28366#pullrequestreview-2077274300)
Thanks @achow101, @s3rk, and @ismaelsadeeq, I hope I addressed all of your comments satisfactorily.

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613854692)
used `exact_target` as proposed

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613782680)
Good question.

It has been a while since I made this change, but if I remember correctly, my fixing the duplicate meaning of `change_cost` broke this bench test due to the following:

Before my change, we would set `change_cost` to `0` as a signal that a transaction does not require a change output. This would happen when a coin selection algorithm found a changeless solution and after creating the recipient outputs there would not be sufficient funds to make a viable change output. However
...

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613679062)
Adopted the proposed change, but those lines get removed in the next commit anyway.

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613840073)
Yes! Thanks, I adjusted the comment.

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613876620)
I introduced an `exact_target` variable above this test block and used it everywhere it applies

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613825421)
Thanks, the "must be 0 if there is no change" is indeed outdated. I removed it.

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613862525)
Added a calculation comment to illustrate the situation:

```code
// = (2 * fee) - (2 * (fee + large_fee_diff)) + change_cost
// = (2 * 100) - (2 * (100 + 90)) + 125
// = 200 - 380 + 125 = -55
```

and asserted that my calculation is correct.

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1613850265)
Pulled `exact_target` out as another variable

💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-2130175396)
> I'm not sure whether [44db79b](https://github.com/bitcoin/bitcoin/commit/44db79b0c48d6b1a26dba6ab01c2a296d610c06b) is useful since it's fully replaced by the following commit.

Ah oops, yeah I guess the first commit was a vestige of my first attempt at this. I squashed it into the second commit.

Diff of before reviews to latest: https://github.com/bitcoin/bitcoin/compare/e95b9159380f2de7f9a6e7a202cc171ad285ee6c..2c18b86

💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2130187888)
@instagibbs I've expanded the explanation. Happy to elaborate more if things are unclear.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613903504)
The chrono timepoint/duration stuff kind of confuses me. Would this be the idea? (assuming `timeout` is per retry). `sock.Wait` takes milliseconds while the steady clock returns microseconds, so the cast seems unavoidable.
```diff
}

// Wait for response(s) until we get a valid response, a network error, or time out.
- while (true) {
+ auto cur_time = std::chrono::time_point_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now());
+ auto
...

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613915882)
Could do (i think 3?) memcmp here if `is_trivially_copyable_v`, but maybe it's not worth the complexity of differing offsets.