💬 maflcko commented on pull request "msvc: Compile `test\fuzz\bitdeque.cpp`":
(https://github.com/bitcoin/bitcoin/pull/29983#issuecomment-2082404311)
lgtm ACK 774359b4a96d2724dc70f900cb71e084a77164da

💬 josibake commented on pull request "rename bitcoin.conf in dist tarball":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2082429501)
> Sure, if the outcome is "do nothing" that's optimal from our perspective. Enough serious issues...

+1.

Per https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063942430, it might make sense to revisit our tarball structure in the future and be in line with best practices for "installing by untarring into `/usr/local`", but I think that's a separate discussion so I'd suggest closing this for now.

🤔 hebasto reviewed a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2028133161)
I've tested 796a271f0f4b57f61f357aac3e7a76072fed6f9a on Ubuntu 23.10 using a patch from #29960.

There are a few differences between Autotools and CMake builds:

1. In CMake, the resulted archive lacks object files from the following sources:
```
gssapi_client.cpp
gssapi_mechanism_base.cpp
gssapi_server.cpp
vmci_address.cpp
vmci_connecter.cpp
vmci_listener.cpp
vmci.cpp
```
2. CMake build is effectively compiled with `-O3` flag.

3. CMake adds `-DZMQ_CUSTOM_PLATFORM_HPP`.

💬 maflcko commented on pull request "test: Assumeutxo: snapshots with less work should not be loaded":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-2082445522)
> For detailed information about the code coverage, see the [test coverage report](https://corecheck.dev/bitcoin/bitcoin/pulls/29428).

@aureleoules @adamjonas Looks like this is down again. ` LambdaTimeout - 2024-04-29T11:09:51.844Z 3957d9f6-7e67-4e79-bf45-9569b4491cd0 Task timed out after 10.01 seconds `

💬 maflcko commented on pull request "test: Assumeutxo: snapshots with less work should not be loaded":
(https://github.com/bitcoin/bitcoin/pull/29428#discussion_r1582885196)
Why is this removed? The block is in the most-work chain. I think this comment meant a block that is *not* in the most-work chain, no?

💬 hebasto commented on pull request "depends: Fix build of Qt for 32-bit platforms with recent glibc":
(https://github.com/bitcoin/bitcoin/pull/29985#issuecomment-2082485060)
My Guix builds:
```
x86_64
7bd0bf18cb3b17ee5829ca24e7488d0d022e280d0f56ff04c20f9199f787178a guix-build-2e266f33b5d2/output/aarch64-linux-gnu/SHA256SUMS.part
9c07a926fbdafff0f189c454652ccf9028e4ba9c44826b7874affc69bc5b5ae9 guix-build-2e266f33b5d2/output/aarch64-linux-gnu/bitcoin-2e266f33b5d2-aarch64-linux-gnu-debug.tar.gz
8d9ce23406bbcc33f2cc4147f06c715c44c5b0cc18af6be296862c3e531f3f97 guix-build-2e266f33b5d2/output/aarch64-linux-gnu/bitcoin-2e266f33b5d2-aarch64-linux-gnu.tar.gz
34a37952
...

💬 Sjors commented on pull request "[do not merge] validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2082492618)
I switched to the halving block! Also dropped all prerequisites but #26045 from the description, but see conceptual discussion in #29616.

⚠️ Sjors opened an issue: "Pause IBD during AssumeUTXO snapshot load"
(https://github.com/bitcoin/bitcoin/issues/29993)
### Please describe the feature you'd like to see added.

The `loadtxoutset` RPC call should cause IBD to pause until the snapshot is loaded and the snapshot chain is activated.

### Is your feature related to a problem, if so please describe it.

When I start a fresh node and load a mainnet shapshot (#28553) any progress is completely buried in the log.

More importantly, I get the strong impression, though haven't properly tested this, that the IBD process slows down the snapshot load. This
...

💬 Sjors commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2082496290)
Definitely not a blocker: #29993

✅ willcl-ark closed an issue: "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf"
(https://github.com/bitcoin/bitcoin/issues/29139)

💬 willcl-ark commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-2082504824)
I agree, going to close this issue and #29903 for now

✅ willcl-ark closed a pull request: "rename bitcoin.conf in dist tarball"
(https://github.com/bitcoin/bitcoin/pull/29903)

🤔 BrandonOdiwuor reviewed a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2028283852)
Concept ACK

🤔 hebasto reviewed a pull request: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2028305896)
Concept ACK.

👍 willcl-ark approved a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2028133549)
ACK 38e24dfc5f1c54723018bf9d973b7d9cf9a2b824

I think the macro simplication included in this change is a nice win for simplicity.
Ran all the updated examples in contrib/tracing, and played around with adding/removing various tracepoint arguments to check everything works as expected.

Left one q and one suggestion inline, but neither a blocker for this.

💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1582972621)
In: 9343ee83ec9ca1ac32d9686f6aea6d755c623562

It's not clear to me how this differs from the semaphore gating being introduced. ISTM that the caller will need the semaphore in either case, so what difference does preparing the argument inside this `if` make? The `TRACEPOINT` macro appears to expand to an `if(TRACEPOINT_ACTIVE( ...`

💬 willcl-ark commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1582872788)
In: 38e24dfc5f1c54723018bf9d973b7d9cf9a2b824
If you re-touch these, it might be nice (user-friendly) to add a simple check that we are running as root. Currently without root you see:

```log
Hooking into bitcoind with pid 4039799
Traceback (most recent call last):
File "/home/will/src/bitcoin/contrib/tracing/p2p_monitor.py", line 258, in <module>
main(pid)
File "/home/will/src/bitcoin/contrib/tracing/p2p_monitor.py", line 124, in main
bitcoind_with_usdts.enable_probe(
Fi
...

💬 paplorinc commented on pull request "fuzz: don't allow adding duplicate transactions to the mempool":
(https://github.com/bitcoin/bitcoin/pull/29990#discussion_r1582993565)
Considering the purpose of fuzzing is to test the combination of valid states, shouldn't we include tests that involve sending duplicate transactions to the mempool?

Furthermore, since the proposed change affects how the `add_to_mempool` flag behaves, would it be more transparent to redefine it to directly reflect whether a transaction can be added to the mempool, i.e. `bool add_to_mempool = !pool.exists(GenTxid::Txid(tx->GetHash())) && fuzzed_data_provider.ConsumeBool();`?

👍 brunoerg approved a pull request: "validation: don't clear cache on periodic flush: >2x block connection speed"
(https://github.com/bitcoin/bitcoin/pull/28233#pullrequestreview-2028345200)
crACK 4a6d1d1e3bc3d559cd13d134b4b827f22635ac4b

🤔 stickies-v reviewed a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668#pullrequestreview-2028288474)
nit: I think the last 3 commits should be squashed