💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745827757)
I think one of them may have been my initial version, but I changed it because I didn't like the `else` and assigning the bool from itself (inverted).

So I'll leave this as-is for now.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745847260)
> build failure

Hmm. Pushed something to work around the msvc build failure.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745847386)
Actually, done here, because it is just one line of code :sweat_smile:

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745847825)
For symmetry, but removed in the last push.

👍 stickies-v approved a pull request: " bench: Remove redundant logging benchmarks "
(https://github.com/bitcoin/bitcoin/pull/30790#pullrequestreview-2283574260)
ACK fadbcd51fc77a3f4e877851463f3c7425fb751d2

Logging conditionality is orthogonal to the performance effect of logging threadnames, so just keeping the unconditional one makes sense. Removing the now unused `LogPrintfCategory` is a nice cleanup.

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1745868679)
> the "string constructor" tests to be re-added

That'd be my preference for now. Even if the tests are redundant, it seems cleaner to keep them for now to make review trivial (and maybe for symmetry with the uint256 tests).

There are many more obvious redundant and useless tests, like `BOOST_CHECK(1 == 0+1);` in this test case, so a lot more could be changed, but maybe removing tests can be done in a follow-up, or not at all.

But none of this is a blocker, so fully up to you. Feel free
...

👍 pablomartin4btc approved a pull request: "test: Add coverage for dumptxoutset failure robustness"
(https://github.com/bitcoin/bitcoin/pull/30817#pullrequestreview-2283624689)
cr & tACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

💬 ryanofsky commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1745887633)
The check above reminds me of the part of the XZ backdoor where the attacker inserted a single . character to disable a linux security feature in a similar cmake check:

https://git.tukaani.org/?p=xz.git;a=blobdiff;f=CMakeLists.txt;h=d2b1af7ab0ab759b6805ced3dff2555e2a4b3f8e;hp=76700591059711e3a4da5b45cf58474dac4e12a7;hb=328c52da8a2bbb81307644efdb58db2c422d9ba7;hpb=eb8ad59e9bab32a8d655796afd39597ea6dcc64d

So I think it might be useful to add a sanity check to confirm that `cxx_supports_forti
...

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2332233857)
@sdaftuar Added your benchmarks as a commit to this PR (I've reworked it to auto-detect the transaction count).

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745897934)
Taken, thanks.

💬 fanquake commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1745898836)
I am certainly open to additional approaches in regards to the (potential fragility) of online source.

> So I think it might be useful to add a sanity check to confirm that cxx_supports_fortify_source is actually true in release builds:

I have a PR to try and do this in #27038.

👍 ryanofsky approved a pull request: "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type"
(https://github.com/bitcoin/bitcoin/pull/30824#pullrequestreview-2283657342)
Code review ACK 30803a35d54acda19ded88474c205f8954fea5e1

💬 ryanofsky commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1745902560)
> I have a PR to try and do this in #27038.

Nice that seems potentially more reliable than suggested sanity check

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745904431)
Will comment elsewhere as well, but I just updated this limit to be a limit on the number of distinct clusters that are conflicted.

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745906461)
Taken

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745907045)
Taken

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745907713)
Removed

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745908827)
Deleted.

💬 instagibbs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745889618)
I think it's clearer to name this `p2p_headers_presync` as well as the file.

💬 instagibbs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745881799)
won't this always build off genesis?