💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1752157540)
Nevermind, I just force-pushed a better solution. No need to shuffle anymore.

💬 brunoerg commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2341166441)
Force-pushed with a small improvement on fuzz target (https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1752007850)

💬 hebasto commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2341174848)
> I managed to get bitcoind compiled on my windows installation (Windows 10 Pro 22H2, MSVC 2022), didn't observe a notable difference in the benchmarks.

I hope everyone measuring performance on Windows has disabled all antivirus software, at least for the data directory. Right?

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