💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716610850)
Nice!

Alternatively, since we claim to be testing whitespace support - and we've already tested that non-whitespace values are parsed correctly -, we might as well compare `ParseHex("12 34 56 78")` to `ParseHex("12345678")` instead - otherwise all tests would fail all the time when an error is introduced, this way only the appropriate tests would fail (+ testing is simpler).

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716685019)
Could we simplify some of these declarations (at least in tests) to e.g.
```suggestion
auto expected{ArrayFromHex("971a55")};
```

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716622610)
Can me modernized/simplified:
```suggestion
std::vector<unsigned char> expected(std::begin(ParseHex_expected), std::end(ParseHex_expected));
```

(nit: weird `ParseHex_expected` naming)

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716705665)
Do we even need the vector conversion here?
```C++
return CScript(data.begin(), data.end());
```

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716709558)
Checked that `ScriptFromHex` is only used for non-static values now 👍

Though I'm not sure `ToScript(ArrayFromHex("0302ff03"))` is more readable (or performant) than `ScriptFromHex("0302ff03"`.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288502723)
> Are these just bugs with the implementation here, or an issue with CMake?

That's by CMake design.

> However, with CMake, it doesn't seem to figure out what order to do things in. i.e it just tries to run the tests, without compiling them:
>
> ```shell
> cmake -B build
> cmake --build build --target test
> Running tests...
> Test project /bitcoin/build
> Connected to MAKE jobserver
> Start 1: util_test_runner
> 1/133 Test #1: util_test_runner .....................*
...

💬 dergoegge commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288512730)
> Up to you, but there is a small benefit of going with the fuzz tests first (if possible)

It looks like the tests here mostly operate on the split out modules after the refactoring, as opposed to a implementation agnostic test through the p2p interface (which would be a giant pita to write). So splitting the fuzz test out would be fine as they don't directly assert that no behavior changes are made. It would of course be sad if we split them out and then only the refactoring lands (which has
...

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288526278)
> The test target is generated by CMake. Such targets [cannot](https://cmake.org/cmake/help/latest/command/add_dependencies.html) be subject to top-level target dependencies.

Ok. So what about `deploy`? If it's a custom target of ours, it should know to build `bitcoin-qt` first?

🤔 marcofleon reviewed a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2237934699)
Tested ACK fa198d1a196482bf363ba57a3f4d6d2d860b8ce5

🤔 stickies-v reviewed a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2237840417)
Concept ACK for more compile time validation.

Strangely enough, 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 doesn't compile for me even though CI seems fine.

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716732991)
nit: while touching, might be worth updating to

```suggestion
assert(bytes.size() == m_keydata.size());
```

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716752262)
I'm unable to compile 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 using:

```
% clang --version
Homebrew clang version 17.0.6
Target: arm64-apple-darwin23.3.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin
```


```
git clean -xdf && git checkout 09458eadc9a4484ba37a70d1b378ed3f3c9e31d0 && ./autogen.sh && ./configure --without-gui && make -j 7

./uint256.h:131:19: error: call to consteval function 'ConstevalHexDigit' is not a constant expression
auto lo = Const
...

📝 hebasto opened a pull request: "guix: Drop unused module from manifest"
(https://github.com/bitcoin/bitcoin/pull/30653)
My Guix build:
```
x86_64
170df52c2238510bd166f3fb1c4c3c11d2c1480a2e468fd532cb4d0435ac11cf guix-build-c7fb80a08f98/output/aarch64-linux-gnu/SHA256SUMS.part
54e71ef5135464f58e3db4a3b893fa2f26a2c9cfb465699a363bb59a0d1bd94f guix-build-c7fb80a08f98/output/aarch64-linux-gnu/bitcoin-c7fb80a08f98-aarch64-linux-gnu-debug.tar.gz
806d6042151e0af026748379b9bbbfea53d4c91555b2f0d05ed11faf83f429bb guix-build-c7fb80a08f98/output/aarch64-linux-gnu/bitcoin-c7fb80a08f98-aarch64-linux-gnu.tar.gz
96f111f81
...

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2288577940)
> > to you, but there is a small benefit of going with the fuzz tests first (if possible)
>
> It looks like the tests here mostly operate on the split out modules after the refactoring, as opposed to a implementation agnostic test through the p2p interface (which would be a giant pita to write).

Indeed, all of these tests require the refactors (on master we'd have to create a chain to fire validation events + construct actual transactions with each type of invalidity).

> So splitting th
...

💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716833566)
What is your macOS and XCode version? IIRC Xcode 14 is no longer supported, see also https://github.com/bitcoin/bitcoin/pull/29934, but I think this isn't documented, so probably up for debate.

The CI passes, because it is using Xcode 15, see d742be3d3f5d5063d7160f72422bce2fec953f38

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716839513)
It was working for me with:
```
% clang --version
Homebrew clang version 18.1.8
Target: arm64-apple-darwin23.5.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm/bin

% xcodebuild -version
Xcode 15.4
Build version 15F31d
```

👍 vasild approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2238035999)
ACK b31a0cd0378184b2b9eb8f4bd3120cbd32c62005

💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288632385)
Thanks for the review! Obviously this is fine to merge, but I am still unhappy about the average performance increase, which isn't 100x, but closer to the worst case performance increase (1.35x). I'll push once more, sorry.

🤔 BrandonOdiwuor reviewed a pull request: "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate"
(https://github.com/bitcoin/bitcoin/pull/30636#pullrequestreview-2238068310)
Concept ACK

💬 marcofleon commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2288658870)
Sounds good, I'll review again when it's improved then. Looked to be about 2x when I just tested it.

🤔 furszy reviewed a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2238083184)
ACK 8f2522d