💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742282984)
My comment is about the .cpp changes though (I wanted to mention I don't love, but am okay with, the header move changes).

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742287072)
The IDE marked these as unused, but since you say there's logic behind the current structure, I've reverted them

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742287290)
Done, thanks!

⚠️ fanquake opened an issue: "cmake: incorrect assumption that `Debug` build type will not use optimisations"
(https://github.com/bitcoin/bitcoin/issues/30800)
CMakeLists.txt currently states:
https://github.com/bitcoin/bitcoin/blob/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8/CMakeLists.txt#L487-L491

However that isn't always true. If, for example, depends is built with `DEBUG=1`, and CMake is configured with `-DCMAKE_BUILD_TYPE=Debug`, `-O1` will actually be used. This is confusing, not just because the documentation is incorrect, but when making a change to [switch the build type to `Debug` in oss-fuzz](https://github.com/google/oss-fuzz/pull/12443)
...

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742287728)
I don't think `valid_hex_64` is so short-scoped? Sure, you can manually validate that the 9 places where it's used don't mutate it, but why not make it explicit and fool-proof with `const`?

💬 fanquake commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742287815)
Issue in #30800.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742287998)
Done (without the const part, which just seems like noise in this context to me, but am open to changing my mind about it)

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742288290)
`gmake` is an accepted alias for `make` when it refers to GNU Make, which is required to build depends.

Also please refer to https://packages.ubuntu.com/noble/amd64/make/filelist.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742289121)
> I think this can be dropped from the commit message

Done, your right, the formulation wasn't the best

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742290292)
The iwyu changes were a bit excessive, but kept the ones that are related to this PR, thanks!

💬 ryanofsky commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742290676)
Good point that "overridden" is probably more appropriate word, but I think I already understood that this meant overridden.

The thing I don't understand is what causes MSAN_FLAGS to be overridden when FLAGS_DEBUG variables are unset, but not overridden when they are empty? Appreciate the clarification. I'm not asking for any particular reason, just curious to understand this better.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742291673)
I don't recall installing Wine.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742291832)
Pushed some changes, let me know if it's any better, or if you insist on placing them in the cpp file in a different place than the header (with some explanations)

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742293570)
Isn't that just a very expensive way to do `assert(random_hex_string == ToLower(result->ToString())`? (and also, that's not `hex` testing, but `uint256` testing)

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742293594)
Fair, added const

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742294557)
Used the `@{` and `@}` formats

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742294935)
Use the version in my edited comment above

💬 achow101 commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2326857491)
Anyone have an asic want to reorg that?

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742295498)
nice! changed

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742295827)
Lack of fuzzing engines for MSVC?