💬 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

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