💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742269284)
I don't really see the value in doing that for such short-scoped test variables - but you can convince me with good arguments, it just seems like noise to me in these cases

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742272526)
those were testing the same input, this one is testing the parsing of `result->ToString()`

🤔 maflcko reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2245305890)
Submitting my review comments, now that this has been merged.

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1725182221)
d2fda82b4954f4af7e7d340cf42b9cb34d96cde1: 4.2.2 Looks ancient, so maybe it can be dropped already?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1724779168)
nit in 3118e40c6157c8ab9e264518d1065d2b0fc07795: The comment seems wrong? This is an internal lib, neither stable nor "compatible"

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1723106474)
nit in commit https://github.com/bitcoin/bitcoin/commit/4a0af29697b62d32af6f60d3ec70cd2ed4d7243c ("cmake: Add TryAppendCXXFlags module"):

could it make sense to mention that `IF_CHECK_FAILED` is never applied to the linker flags, whereas `IF_CHECK_PASSED` seems to be? (`IF_CHECK_FAILED` is unused dead code, so nothing blocking)

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726684296)
3d853795707c5a1828dcd09c1f68bb07dee472cd: Why does this need special handling for msvc? What problem is this trying to solve?

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726761710)
57a6e2ef4abbfd2b12ee6489366bc6609bead263, two questions:

* Looks like Network was added in 4af4672525f698c7b491023fcd36a17a4e982070, but not guarded by the wallet
* Why is this needed by the wallet?

💬 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)