💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719107583)
In commit "refactor: add util::HexLiteral and util::Vec using statements" (ced7cec1c6d79159f8212d3a92a8f7583ef11884)

It seems like there is a problem with this on windows, looks like because begin and end are returning `std::_Array_const_iterator` types, instead of character pointers. Following change might fix it:

```diff
- return {array.begin(), array.end()};
+ return {array.data(), array.data() + array.size()};
```
Error is:

https://github.com/bitcoin/bitcoin/actions/runs/1
...

💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719107427)
nit: You could move this to the top of this block like with the other cases (unless you duplicate anyway)

💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719109497)
nit: The three lines are similar to above for n1, so could be put into a function together

💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719103180)
You can simplify this by just returning the result from the function and then doing these checks respectively in the two places where the function is used now. Should drop the `test_` from the name as well then.

💬 fjahr commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719128993)
Sorry for the long wait, I finally got around to look at your branch @alfonsoromanz . You are looking at this `wallet_last_processed_block` but that is coming out of node 0. When you load the wallet into node 2 it doesn't know anything about it, so this can be ignored. What you might be thinking of instead is the birthdate of the wallet, that's part of the backup afaict so that should be relevant. But the birthdate is earlier than 299 because the wallet seems to be created before a bunch of bloc
...

💬 tdb3 commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2292561601)
Built and ran tests successfully with 67b1e236334f38ec4e4d2251dbdfb790f20ed88b
More details:

Debian 12.6
cmake version 3.25.1
gcc/g++ (Debian 12.2.0-14) 12.2.0

```
cmake -B build
cmake --build build -j18
ctest --test-dir build
```

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719324036)
Is https://github.com/bitcoin/bitcoin/pull/29369 related or possibly a fix?

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719345193)
style nit in 84c830f27fd62db4a9cb93bf6d28a86f7751e504: Could use `std::byte` as default? (See below for reasoning)

(Reply to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719000295)

I think your link should say https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716903532

This is a newly introduced function, so I think changing it should not extend to untouched files.
To clarify, I am not saying that you should switch callers to use `std::byte`, only the default.

...

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719367069)
> It's because part of my goal with this PR was to etch these bytes into the runtime binary. Things that can be done at compile time without too much hassle should be done at compile time IMO.
>
> If we go the `Vec(HexLiteral` route at least it becomes shorter.

Makes sense, thanks for looking into it!

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1719382540)
Sure, fixed the style

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#discussion_r1719384643)
Sure, removed the ranges include.

(Maybe another reason to enforce iwyu in the CI)

💬 Sjors commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2292976404)
@maflcko for this test I set `enforce_BIP94 = true` for regtest. I just updated the branch to set `nPowTargetTimespan` to 144.

@TheBlueMatt feel free to take all or some of that code. Otherwise I'll open a followup.

💬 maflcko commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2293002597)
> `enforce_BIP94 = true` for regtest

I guess it is fine to do, but if this change is taken, it would be good to add a short release note snippet with a `Tests` section about this change. Otherwise people may wonder why their fancy regtest chain isn't working anymore due to a testnet4 rule (`time-timewarp-attack`). (Also, the code comment could be updated to say testnet4/regtest)

💬 Sjors commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2293028876)
@maflcko added

💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719562102)
```
Filesystem Size Used Avail Use% Mounted on
/dev/nvme0n1p2 468G 110G 335G 25% /
```
It worked with gcc. `./configure` didn't like libc++-17. After reboot I can't get it to fail anymore (also I don't mean to hold up this PR)

💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#issuecomment-2293114610)
ACK faa1b9b0e6d

💬 maflcko commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1719563783)
q in f1c1b37d2247eed49156c0467daa68aa38497bb8: Can you explain why this change is correct? `SeedRandomForTest` is doing two things:

* Set the internal global RNGState
* Reseed the passed `g_insecure_rand_ctx`

Removing `g_insecure_rand_ctx` does not remove the need to make the fuzz tests deterministic and stable.

I'll drop this change as well.

💬 maflcko commented on pull request "test: [refactor] Use g_rng/m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1719563882)
> so it could still be used conveniently to seed m_rng.

Not sure. The "feature" is only used in three places, two of which will be removed in the last commit.

So it just seems easier to drop this change and instead move the call to `Reseed` from this function to the only place that needs it.

📝 fanquake locked a pull request: "3 more security enhancements"
(https://github.com/bitcoin/bitcoin/pull/30663)
Please see the detailed description in the commits below.

📝 fanquake locked a pull request: "Adding security"
(https://github.com/bitcoin/bitcoin/pull/30662)
Adding two security enhancements. Please see the detailed description in the two commits below.
Thanks.