💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716887365)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716685019:
I'll meet you half-way with `constexpr auto`. :)

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716903532)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716703571:
Tried to start switching to `std::byte` after your comment, but it quickly propagated to untouched files.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717015082)
Attempted to minimize the diff in latest push, but went with `VecFromHex` for string literals and introduced a new `ScriptFromHexStr` for the 2 runtime validated call sites.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716875704)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716678159:

Thanks, better get this part right. :)

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716900164)
Responding to https://github.com/bitcoin/bitcoin/pull/30377/files#r1716752262:

Weird, actually works for me on same clang version as @stickies-v, but under Linux:
```
$ clang --version
clang version 17.0.6
Target: x86_64-unknown-linux-gnu
```
Maybe it's the standard library that is somehow different (missing some `constexpr`)?

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717012368)
> `ScriptFromHex` failed for empty inputs.

How come? `ParseHex("")` returns `0` as verified by **util_tests.cpp**. Besides that, I think it's not really what we are testing in this file.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717314517)
Taken in latest push.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717316492)
Taken in latest push.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717317988)
Taken in latest push.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717318999)
Taken in latest push, should add co-authorship on next re-touch.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717320370)
Taken in latest push.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717327999)
Updated with slightly different string in latest push and removed comment about consistency above as well.

```C++
throw "Only lowercase hex digits are allowed, for consistency";
```

💬 hebasto commented on pull request "test: Fix test log file name":
(https://github.com/bitcoin/bitcoin/pull/30654#issuecomment-2289467414)
> This instead seems to be a reversion to prior behaviour to facilitate something CMake related. I'm curious why this has only become an issue now, given we've already got the tests in CMake? Or is this something that was missing and is being ported now?

I had some concerns while documenting CTest behaviour in https://github.com/hebasto/bitcoin/pull/329.

📝 achow101 opened a pull request: "kernel: pre-28.x chainparams and headerssync update"
(https://github.com/bitcoin/bitcoin/pull/30658)
Update chainparams and headerssync parameters for the pre-28.x branching, per https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717382220)
New version of the comment in latest push:
```
* @warning Unlike VecFromHex which returns a vector, the returned array may use
* significant stack space if called inside a function.
```

👍 ryanofsky approved a pull request: "refactor: Replace ParseHex with consteval ArrayFromHex"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2238863495)
Code review ACK 65bc6dcd573fe74d2ce175466c30ed830d17f0fc. Looks good, thanks for working on this, and I think this will avoid the need to add the ParseHex suppression in #30415. Left a few comments but nothing critical.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717352300)
In commit "test refactor: util_tests - Use BOOST_CHECK_EQUAL_COLLECTIONS()" (c6e1d5bff44b7d382a5a91b6722743b5fe07a95d)

Might be worth mentioning this change in commit message or saying it has other test cleanups.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717386520)
In commit "refactor: Hand-replace some ParseHex -> ArrayFromHex + VecFromHex" (8ad7e7d3fdfed4b126859dd83ab81cf8a8ae82a9)

It's not obvious why an extra ToByteVector conversion is necessary here and why ArrayFromHex is not sufficient. Does CScript not accept std::array, or does it not accept uint8_t? I think this change is ok but it would be good to say in commit message what is happening here and how we could simplify this in the future, assuming it is something we should be able to clean up l
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717373651)
In commit "refactor: Add consteval ArrayFromHex and VecFromHex" (b997d266e549ad6745d01a696cd9fbf1d3525944)

It seems like this is assuming the last character in the string is \0 and then ignoring it? I guess that is good because it makes this convenient to call, but in theory it means somebody could call this with a character array that is does not end in \0 and then the last character of the string would be ignored.

Would suggest changing this to:

```c++
size_t size{hex_str[Size-1] ==
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717392937)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717382220

> New version of the comment in latest push:

Thank you! That seems accurate now.