💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1721657604)
nit in https://github.com/bitcoin/bitcoin/commit/a2317e27b7fb86df4e32cd1674c06e09cb808248: Maybe add a comment that this can be removed after `cmake_minimum_required(VERSION 3.25)`?

Alternatively, given that "The default installation of the latest version of MSVC 17.10.4 includes both CMake 3.28.3 and the vcpkg package manager", it may be easier to just require cmake >= 3.25 on MSVC and enforce the policy this way?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1727632112)
You can't run exe on WSL without wine, can you?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1736110362)
in commit a2317e27b7fb86df4e32cd1674c06e09cb808248: This will only print the compiler name, not the flags.

E.g. `-DCMAKE_CXX_COMPILER='clang++;-stdlib=libc++;-m32'` will print the flags nowhere. (This is used in the `ci/test/00_setup_env_i686_multiprocess.sh` task)

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1727630179)
make, not gmake

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742280366)
Yes, in which case we'd be putting an optional (variable) on the right hand side, which goes against what you're saying in the commit message. That's why I suggest removing it, it doesn't help with reviewing this commit imo. It only adds context on why it doesn't do something that was done in another commit (that's not in the tree).

💬 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