🤔 murchandamus reviewed a pull request: "Move maximum timewarp attack threshold back to 600s from 7200s"
(https://github.com/bitcoin/bitcoin/pull/30647#pullrequestreview-2240965068)
crACK 16e95bda86302af20cfb314a2c0252256d01f750

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718764031)
Fascinating. What is your compiler, stdlib version, OS and filesystem for `/tmp/` and `./`?

Does the issue happen when you create a non-`/tmp/` folder? (I presume `/tmp/` is a ramdisk?)

```
mkdir no_ramdisk_temp_prefix
./test/functional/test_runner.py --tmpdirprefix ./no_ramdisk_temp_prefix/ feature_blocksxor

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718767576)
Also, what is `ls --all /tmp/bitcoin_func_test_fj812gkv/node0/regtest/blocks`

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718769752)
Is it Apple https://en.wikipedia.org/wiki/.DS_Store ?

💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718786049)
It's jammy with clang 17, indeed it works with a non-`/tmp` folder.

💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1718816229)
Jammy doesn't have clang 17, so I guess it is installed from the nightly llvm apt and then `CC=clang-17 CXX=clang++-17 ../configure` (using the libstdc++-11, not libc++)?

🤔 maflcko reviewed a pull request: "test: [refactor] Use g_rng/m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2241150302)
Sure, pushed your branch. A skim looks good. I left a nit and will do a real review later.

💬 maflcko commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1718879706)
nit in bb67ca09c88a46b99bf83e937fa761f0e25cba37: Not sure about this change. It seems overly restrictive to require a `FastRandomContext&` here, when a caller may not want to pass anything at all, or a different rng.

I think it would be better to drop this change.

💬 TheCharlatan commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1718552423)
Nit: I find these callbacks a bit ugly, how about:
```diff
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 0b4e0d183a..a30ac53f97 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -54,0 +55 @@ void initialize_chain()
+ .min_validation_cache = true,
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index cf47d16faf..62ff61b227 100644
--- a/src
...

👍 TheCharlatan approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2240627537)
ACK fa69afd51879f8441df7a51144318a49e7dd27b9

The logic is a bit convoluted, but I don't have many suggestions for improving it and I think it is important to get this performance improvement deployed quickly.

💬 TheCharlatan commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1718893373)
Nit: The includes need to be corrected again.

💬 mzumsande commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1718921534)
I see similar size to @Sjors, as of height=2873716:
```
du -sh ~/.bitcoin/testnet3/chainstate/ ~/.bitcoin/testnet3/blocks/
13G /home/martin/.bitcoin/testnet3/chainstate/
82G /home/martin/.bitcoin/testnet3/blocks/
```
(I wasn't continuously online during the fork shenanigans, so having fewer blocks makes sense).

📝 hebasto opened a pull request: "CMake replaces Autotools"
(https://github.com/bitcoin/bitcoin/pull/30664)
This PR is based on https://github.com/bitcoin/bitcoin/pull/30454 with additional commits that delete both the Autotools-based build system and the legacy MSVC build system.

It aims reveal conflicts with other PRs.

For more details, please refer to today's IRC meeting discussion.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993291)
fixed

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718993817)
fixed (as part of one of the larger changes)

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718995086)
Adopted the type approach you suggested so this is resolved as well with that.

💬 ryanofsky commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1718994222)
> nit in [bb67ca0](https://github.com/bitcoin/bitcoin/commit/bb67ca09c88a46b99bf83e937fa761f0e25cba37): Not sure about this change. It seems overly restrictive to require a `FastRandomContext&` here, when a caller may not want to pass anything at all, or a different rng.

Makes sense. I hadn't really looked at this function closely and was just trying to keep behavior unchanged. Maybe it would be good to change signature to something like:

```c++
SeedRandomForTest(SeedRand seed = SeedRand:
...

👍 ryanofsky approved a pull request: "test: [refactor] Use g_rng/m_rng directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2241332588)
Code review ACK f1c1b37d2247eed49156c0467daa68aa38497bb8. Current version of this looks identical to the branch I pushed, except it is rebased and whitespace in the scripted diff was adjusted.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1718998488)
Thanks, I agree that the default wasn't very sensitive when looking at it this way. I have adopted the type approach that @ryanofsky suggested. I think it may be a bit more complicated to understand initially for users but it's also easier to hand because you don't have to deal with a hash or height in the most common use cases.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1719001528)
I have introduced your suggested fix as a separate commit because the main commit is already quite big and I think it's easier to review this way. I added some minor documentation changes since `CreateUTXOSnaphot()` is now only used in tests.