💬 vostrnad commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2341236503)
@hebasto Yes, I have disabled antivirus for both the data directory and the directory where the binaries are located, I checked that the antivirus CPU utilization stays low during the benchmarks, and I'm using identical directory locations for both versions so they wouldn't be treated differently by the antivirus anyway.

💬 fanquake commented on pull request "build: Use CMake's default permissions in macOS `deploy` target":
(https://github.com/bitcoin/bitcoin/pull/30838#issuecomment-2341237191)
Guix build (aarch64):
```bash
8eeb195889318631934b31746dcbd04b26c2312e2aa29dbd7be9de9581a8b805 guix-build-5ba03e7d35e1/output/arm64-apple-darwin/SHA256SUMS.part
05c10866f6972aa58a379935dbc4eb39a8e2abc54a27aa4fa2ce06a7443593ba guix-build-5ba03e7d35e1/output/arm64-apple-darwin/bitcoin-5ba03e7d35e1-arm64-apple-darwin-unsigned.tar.gz
f81ec9368bf068a373ec88428becdef08533e8984ad95a3b017f7f1a8cbbb5b1 guix-build-5ba03e7d35e1/output/arm64-apple-darwin/bitcoin-5ba03e7d35e1-arm64-apple-darwin-unsign
...

💬 hebasto commented on pull request "build: Use CMake's default permissions in macOS `deploy` target":
(https://github.com/bitcoin/bitcoin/pull/30838#issuecomment-2341254274)
My Guix build (aarch64, for both `umask=0002` and `umask=0022`):
```
8eeb195889318631934b31746dcbd04b26c2312e2aa29dbd7be9de9581a8b805 guix-build-5ba03e7d35e1/output/arm64-apple-darwin/SHA256SUMS.part
05c10866f6972aa58a379935dbc4eb39a8e2abc54a27aa4fa2ce06a7443593ba guix-build-5ba03e7d35e1/output/arm64-apple-darwin/bitcoin-5ba03e7d35e1-arm64-apple-darwin-unsigned.tar.gz
f81ec9368bf068a373ec88428becdef08533e8984ad95a3b017f7f1a8cbbb5b1 guix-build-5ba03e7d35e1/output/arm64-apple-darwin/bitcoin
...

💬 maflcko commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1752212412)
This still reads a bit odd, because a set shouldn't hold duplicate values, but the loop may produce duplicate values?

Not sure what you are trying to achieve, but `for (const auto& net : ALL_NETWORKS) { if (fdp.Bool()) nets.insert(net); }` may be what you are looking for?

💬 hebasto commented on pull request "build: Use correct variable name":
(https://github.com/bitcoin/bitcoin/pull/30791#issuecomment-2341303315)
> This uncovers a bug in the build system: Building with a fuzz engine fails, because the ctime tests are auto-detected in cmake, based on whether or not valgrind-devel is installed or not.

Thanks! I'll submit a fix shortly.

💬 fanquake commented on pull request "test: autogenerate bash completion":
(https://github.com/bitcoin/bitcoin/pull/30860#issuecomment-2341307714)
https://github.com/bitcoin/bitcoin/actions/runs/10788681404/job/29919981336?pr=30860#step:7:3564:
```bash
Run rpc with args ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-x86_64-apple-darwin/src/test/fuzz/fuzz', PosixPath('/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/rpc')]Error: RPC command "format" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update src/test/fuzz/rpc.cpp.
libc++abi: terminating

Error: RPC command "
...

👍 fanquake approved a pull request: "test: Drop no longer needed workarounds"
(https://github.com/bitcoin/bitcoin/pull/30847#pullrequestreview-2292918815)
ACK 5c80192ff6b982ee3a75be4142fe942b8206f2cd. Looks correct:
```bash
cmake -B build -DENABLE_EXTERNAL_SIGNER=OFF (and commented out `EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED` tests)
cmake --build build
<snip>
Start 74: raii_event_tests
63/133 Test #74: raii_event_tests .....................***Skipped 0.01 sec
Start 99: system_tests
88/133 Test #99: system_tests .........................***Skipped 0.01 sec
<snip>
Total Test time (real) = 80.97 sec

The following
...

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1752244306)
> This still reads a bit odd, because a set shouldn't hold duplicate values, but the loop may produce duplicate values?

Yes, it may produce but would be "ignored" when trying to add.

> Not sure what you are trying to achieve, but for (const auto& net : ALL_NETWORKS) { if (fdp.Bool()) nets.insert(net); } may be what you are looking for?

Yes, will address it.

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1752249009)
Done, thanks.

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1752256496)
I wasn't using depends, so had to update multiprocess on my system.

💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752254628)
nit: I think this would be the same, just shorter?

```suggestion
if (tx->IsCoinBase() || !blockindex || WITH_LOCK(::cs_main, return !(blockindex->nStatus & BLOCK_HAVE_MASK)) ||
```

💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752259940)
Also it seems like the `!blockindex` can be taken out safely since it's checked a few lines above and if the check fails we throw an error.

💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1752270427)
> I think the suggestion doesn't work because we still need to feed the result into have_undo so that if UndoReadFromDisk fails for an unexpected reason like disk errors, we don't attempt to access blockUndo below and segfault.

I see, I think in that scenario it would be best to throw an error right there instead of ignoring it silently and giving the user an incomplete response? I mean, we have all indication that we should have the undo data in that scenario and then we can not read it, it
...

💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2341427105)
I'm going to hold off reviewing this until I see the TxGraph PR to better understand its intended usage. :+1:

⚠️ mcelrath opened an issue: "Race condition between ZMQ UpdateTip and getblocktemplate"
(https://github.com/bitcoin/bitcoin/issues/30862)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

When ZMQ announces a new chain tip via "sequence" "C" message, if `getblocktemplate` is called immediately, it can return a block template that does not reflect the new chain tip.

This is important for Braidpool, which needs to switch to a new chain tip as quickly as possible when a new block comes in.

### Expected behaviour

`getblocktemplate` should return a result consistent with th
...

💬 ryanofsky commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2341480416)
Are there release notes for cmake transation? Seems like the autoconf -> cmake mapping table https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e would be especially helpful to have in release notes to help users and packagers convert previous configurations to new ones.

🤔 stickies-v reviewed a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2291996189)
I'm still a strong concept ACK on this, but I wonder if the incremental appoach currently taken is the best approach. We'll have to (I think quite likely non-trivially) update the logic based on flags, widths, precisions, conversion, ... actually used in the codebase, as seen with e.g. the precision and zero-padding comments?

So perhaps it'd be sensible to first create a branch that forces `ConstevalFormatString` everywhere, informing our requirements and adding full unit test coverage, and t
...

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751671633)
Note: it seems we are explicitly preventing `*` dynamic width/precision specifiers here, which was previously allowed (and checked) with the linter. That's not necessarily bad, it seems we're not currently using them (quick non-exhaustive search) and it can of course be added back in later, and we're throwing instead of silently ignoring it.

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752314431)
We do have quite a few use cases of `\.[\d+]f` precision that would throw here. No issue in the current code, but would like have to be overhauled in the next PR?

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751669308)
I think having a separate `CountNumberFormatSpecifiers` would have made testing this function a bit easier and with better error feedback, while simultaneously also reducing template instantiation. Neither are overwhelmingly strong arguments, but I don't think there are really any downsides either?

E.g. sample, pretty rough (but passes tests) diff:

<details>
<summary>git diff on faa32adbcf</summary>

```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp

...