💬 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)"

💬 glozow commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819649058)
Move to bottom to retain chronological ordering?

💬 glozow commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819652441)
There is a test for both cases v3 + BIP125 and for v3 + non-BIP125 iirc

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1819663339)
I squashed the change in here: ad95b2c0e21f1e865f967aa9463ef99bc252550a

💬 l0rinc commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1819663675)
> Does changing strprintf to tfm_format_raw here catch any potential issues that wouldn't be seen in manual testing?

By manual testing do you mean automated testing of print statements?

> Clang-format can be helpful for new code, but I don't think it's necessary to re-indent code for it that isn't otherwise touched

Please format affected code, don't leave it unformatted, that's why the current code is so confusing.
```
Coding Style (General)
----------------------

Various coding s
...

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2442487595)
Squashed @naumenkogs fix on @marcofleon comment in ad95b2c0e21f1e865f967aa9463ef99bc252550a, plus added a commit replacing `IsFanoutTarget` with `GetFanoutTargets` so the cache for fanout can be dropped. After this, the fanout targets are computed just once per transaction

💬 l0rinc commented on pull request "ci: Use clang-19 from apt.llvm.org":
(https://github.com/bitcoin/bitcoin/pull/30634#issuecomment-2442503039)
Concept ACK

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819683400)
done

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819683484)
taken

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2442518446)
@glozow tried a version of your suggestion which should be more future proof

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1819693843)
> Please format affected code, don't leave it unformatted, that's why the current code is so confusing.

The indentation wasn't wrong, and in the past, long-term contributors generally or often didn't wish to reformat code in their pulls that wasn't wrong in some dangerous way when newer contributors asked for reformatting in reviews. There has been extensive bikeshedding over what the clang-format setup here should even do. Having these arguments on the same line read left-to-right, which was
...