💬 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.

📝 hebasto converted_to_draft a pull request: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868)
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.

Partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489

After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.hpp`.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745999190)
While the code is slightly simpler (-4 lines, indentation), the error message in case of failure is also simpler:

<details>
<summary>with static assert</summary>

```
bitcoin/src/test/util_string_tests.cpp:16:19: error: static assertion expression is not an integral constant expression
static_assert([] {
^~~~
bitcoin/src/util/string.h:39:34: note: subexpression not valid in a constant expression
if (num_params != count) throw "Format specifier count must
...

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1746006077)
It is not "extra", but required. Consider this output:
```
C:\Users\hebasto>where echo
INFO: Could not find files for the given pattern(s).

C:\Users\hebasto>where cmd
C:\Windows\System32\cmd.exe
```