💬 fjahr commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2341197816)
How about adding an instruction for testing assumeutxo mainnet parameters? Unless I am misremembering I think we left assumeutxo out of the testing guide previously because it was not available on mainnet so it would be good to include it now. For some people it may be taking too long or be too resource intensive so I would suggest to put it at the end as a bonus or something like that.

Here is a high-level description of what I think of for a full assumeutxo test: https://github.com/bitcoin/
...

👍 instagibbs approved a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2292826975)
tested ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

Fails when modifications are made on mainnet and regtest.

Another follow up improvement I'd like is direct assertion that the "chainwork" of the header chain never exceed minimum chainwork, or if that is exceeded, we can drop the assertion that the chain never extends past genesis block.

👍 ryanofsky approved a pull request: "[RFC] Switch and/or align debugging flags (back) to `-Og`"
(https://github.com/bitcoin/bitcoin/pull/29796#pullrequestreview-2292846904)
Code review ACK 296def1f6fd936d72d49ce98f9473b4a5d2f9c4b.

Reading more about this it seems like there are real examples of where -Og hurts debuggability https://github.com/bitcoin/bitcoin/pull/16435#issuecomment-513838273 ("Yes. Please! On macOS, without that fix, debugging with lldb/gdb is impossible.") and earlier in this thread.

Also if the goal is to make depends build and main build more consistent, I'm not sure why they couldn't both use -O0 or if the other problems mentioned with -O
...

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