💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1819507957)
think this is right, you can resolve this

💬 hebasto commented on pull request "depends: Specify CMake generator explicitly":
(https://github.com/bitcoin/bitcoin/pull/31171#issuecomment-2442295413)
My Guix build:
```
aarch64
2b8363e54965bc413159a4dbc371a70e4c8a77976c196c57747917928009ed03 guix-build-e2ba8236715e/output/aarch64-linux-gnu/SHA256SUMS.part
4b0b3358ffe8c136759ff8594e5ea4502a7b9299f3c3b271c65bc3187b832960 guix-build-e2ba8236715e/output/aarch64-linux-gnu/bitcoin-e2ba8236715e-aarch64-linux-gnu-debug.tar.gz
303f29d256c2f31909b8a6ffdbcb2c6fd974a3e188bb6726de369c8746ad4b9f guix-build-e2ba8236715e/output/aarch64-linux-gnu/bitcoin-e2ba8236715e-aarch64-linux-gnu.tar.gz
df579740
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819536386)
pretty sure f497414ce76a4cf44fa669e3665746cc17710fc6 means you can't `Assume` anymore?

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819473572)
minor spelling: OutputGroups

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819508593)
Is this name going to set this portion of tests apart enough to be meaningful? It seems only some of the BnB tests that were present in coinselector_tests have been refactored

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819541160)
This segment of code appears to me to be intended to refactor the make_hard_case exhaustion tests, but with differing amounts (17,18 vs. 14,17) and construction (amount = iterative CENT + i vs. bitshifting?). Isn't there double the coins being created in the make_hard_case? I appreciate that this format is more readable, though it's unclear to me why one should exhaust based on the given coin sizes and quantity. Is this related to src/wallet/coinselection.cpp setting TOTAL_TRIES to 100,000 cons
...

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819476976)
Can we eliminate a call to GetFee via reordering assignment?
```
dcsp.m_change_fee = /*155 sats=*/dcsp.m_effective_feerate.GetFee(dcsp.change_output_size);
dcsp.min_viable_change = /*204 sats=*/dcsp.m_discard_feerate.GetFee(dcsp.change_spend_size);
dcsp.m_cost_of_change = /*204 + 155 sats=*/dcsp.min_viable_change + dcsp.m_change_fee;
```

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819544428)
Worth noting that src/bench/coin_selection.cpp makes a copy of the make_hard_case function (commenting as such), so removal here may lead to confusion unless that benchmark is also updated.

💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2442393053)
Force-pushed to address merge conflict from https://github.com/bitcoin/bitcoin/pull/31042, no other changes.

💬 vicariousdrama commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1819601522)
disregard this comment. I skipped your intent in the description

💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2442428828)
Concept ACK and code review ACK. This needs a more useful commit message though.

I wasn't familiar with this c++17 functionality, and at first glance it looked scary to me (as though we might get multiple `cs_main`s instantiated).
[
According to cppreference](https://en.cppreference.com/w/cpp/language/inline) though, this is a reasonable (if not more correct/modern) approach.

[This stackoverflow question/answers](https://stackoverflow.com/questions/56876624/inline-stdmutex-in-header-file
...

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819634235)
Not intentional! I haven't tried negative yet.

💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2442443044)
Wait, I take that back. It seems like we could still get 2 separate `cs_main`s here: one in the lib and the other in the binary. ACK retracted while I investigate.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819636610)
That would require peerman having access to the txrequesttracker directly, so not really a fan 🤔

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819638684)
Yeah, we don't quit early, so we can't assume that it isn't missing inputs here anymore.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1819638959)
Will fix if I retouch / in the next PR

🤔 glozow reviewed a pull request: "test: enhance p2p_orphan_handling"
(https://github.com/bitcoin/bitcoin/pull/31037#pullrequestreview-2400032155)
light code review ACK 9de9c858d5aa412e1c6086e564fbe85984a4f2d5

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1819654430)
The commit with the casting works, but the test one is actually not a good way of testing this; it would pass both with and without the change.

Turns out that passing a huge value to `IsFanoutTarget` would result in `targets_size` being `0`, which results in the method returning `false`. I don't think there is really a good way of testing this.

I'll take the code commit but skip the addition to the test.

🤔 glozow reviewed a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2400041340)
> `doc/bips.md/` bip125 is still opt-in, we are drifting further and further away from that BIP and will finally basically kill it off with cluster mempool. Not sure what we want to do there.

I think usually if we stop implementing a BIP we can just remove it from bips.md. Or you can say "BIP 125 is not supported. See doc/policy/mempool-replacements.md for replacement policy" ?

💬 glozow commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819648604)
Deprecated means still supported but discouraged
```suggestion
1. (Removed)
```

💬 glozow commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819647758)
That's not what deprecated means, maybe just replace the whole thing with "(Removed)"