👍 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?

💬 instagibbs commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1745887683)
I modified this harness to use `REGTEST` chainparams with a minimum chainwork setting of `0x0` and the harness has yet to fail, fwiw.

Might need to make sure this harness is getting deep enough to actually check what's being intended?

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745913307)
Thanks, took this.

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-2332274663)
Thanks @glozow for the test improvements. I've also made a change to an RBF limit that I think is worth mentioning. Rather than limit the number of direct conflicts that a transaction can have, I've now implemented the limit to be on the number of distinct clusters that the conflicts of a transaction belong to.

The idea is still to create some bound on the amount of CPU we might spend linearizing clusters and doing feerate diagram checks when processing a single transaction. Limiting the n
...

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745941441)
is f94c546ac02ad11ba73174cb8d25220515cee893 the first commit that you can assert optimality? I'm not sure having the benchmark in prior to that is so meaningful