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

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742299214)
Again, here, I don't think the `const` is noise. This variable is passed to multiple functions, and verifying no mutation happens is an unnecessary burden on the developer. Why not just lean on the compiler?

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742300226)
isn't the point of these tests that we shouldn't have know the internals (i.e. black box)?
(though in this case that wouldn't even be true since `random_hex_string` can start with `0x` for `FromUserHex` which isn't added back via `ToString`)

⚠️ TheCharlatan opened an issue: "cmake: Installed static kernel library is unusable"
(https://github.com/bitcoin/bitcoin/issues/30801)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Since the move to cmake, the kernel static library that is installed after a `cmake --install build` is unusable. It basically lacks all symbols, besides those defined in the kernel library target. This was a problem before where some libtool and symbol visibility [issues](https://github.com/bitcoin/bitcoin/issues/25008#issuecomment-1440326006) led to the libsecp static library having to b
...

💬 ryanofsky commented on issue "cmake: incorrect assumption that `Debug` build type will not use optimisations":
(https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2326864201)
Not sure if this makes sense, but if the problem is that specifying _FORTIFY_SOURCE without optimizations triggers a compiler warning, maybe there should just be a check that drops _FORTIFY_SOURCE when it triggers the warning. That might avoid problems if there is a discrepancy between the way our cmake code detects optimizations and the way _FORTIFY_SOURCE implementation does.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742301373)
https://github.com/bitcoin/bitcoin/blob/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8/doc/design/libraries.md?plain=1#L7

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742301624)
K, I don't feel strongly about it, pushed.

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742305814)
I don't think requiring `v` to have implemented `ToString()` is a good approach. Do you see any issues with `return v ? os << *v : os << "std::nullopt";`?

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742312035)
Unfortunately, packages in the main distros still lack CMake configuration files. For example, see https://packages.debian.org/sid/amd64/libzmq3-dev/filelist.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742312732)
I'm fine with that as well, pushed.

💬 fanquake commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742312937)
Regardless of WSL, given these are the general instructions for any Linux, `wine` should be added as a requirement, otherwise the current instructions will fail:
```bash
gmake -C depends HOST=x86_64-w64-mingw32 -j12 NO_QT=1 NO_WALLET=1 NO_ZMQ=1 NO_NATPMP=1 NO_UPNP=1
cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
cmake --build build
bitcoin# wine
bash: wine: command not found
bitcoin# wine64
bash: wine64: command not found
<snip>
ctest --test-dir build
0% tests p
...

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2326882117)
Thanks for the thorough review @hodlinator and @stickies-v, I pushed another batch (first an empty rebase), followed by https://github.com/bitcoin/bitcoin/compare/c6f9add458bb8e142d8ccfe5c661ec56143e3f31..6fa4c92e4c9bf5ea90e25d41b525c88f9c2d834d

💬 stickies-v commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742316356)
I don't insist on placing them in a different place than in the header file, I just don't think this is worth the noise and bigger diff - I don't think anyone uses a declaration's location in the header to find the implementation in the cpp file?

💬 ryanofsky commented on issue "cmake: Installed static kernel library is unusable":
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2326889482)
I wonder if this approach would work: https://stackoverflow.com/a/78099611

Instead of `target_link_libraries(bitcoinkernel bitcoin_crypto)` do `add_library(bitcoinkernel $<TARGET_OBJECTS:bitcoin_crypto>`.

I'd also wonder if it's really necessary to build a combined library, or better to just install the individual libraries we already have.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742319223)
I think it makes it slightly more aligned with the header file, while the diff isn't a lot heavier.
What do you think @hodlinator?

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1742321442)
> Is it even possible for this to be false, given that we skip flush on first run?

Can we logically determine it will never be false? If so, we can simplify this quite a bit.