💬 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

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745945489)
also seems like the assertion fails for me at that commit

💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745962506)
Nice catch. Apparently the example graphs were obtained by finding 100k-1M iterations-optimal clusters *with LIMO* (so while passing in an already-good linearization as input), while the benchmarks didn't pass in an existing linearization.

@sdaftuar is going to construct new ones.

👍 ryanofsky approved a pull request: "test: Pin and document TEST_DIR_PATH_ELEMENT, SeedRand::FIXED_SEED"
(https://github.com/bitcoin/bitcoin/pull/30748#pullrequestreview-2283763614)
Code review ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534

IMO, `TEST_DIR_PATH_ELEMENT` would be clearer as `TESTS_DIR_NAME` and `"test_common bitcoin"` would be clearer as `"bitcoin tests"` but current names seem fine too.

💬 ryanofsky commented on pull request "refactor: Avoid wallet code writing node settings file":
(https://github.com/bitcoin/bitcoin/pull/22217#issuecomment-2332366356)
This PR caused an unintended change in behavior. Before this PR, passing `-nowallet=0`would cause a wallet "1" to be loaded. After this PR, it triggers a vague "JSON value of type bool is not of expected type string" error and uncaught exception. #30684 improves this by avoiding the exception and making the error clearer.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745986533)
These alternatives work like a simple state machine, close to how we would look at the code: are we inside a format (i.e. started with %) or outside, switching states between the two. The second suggestion can even be used to determine is the number of params is more or if it's less than the desired one.