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

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1742323569)
Yeah we could rename `fDoFullFlush`. What about `should_write`?

💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1742293140)
In 8cfaf6e6ef6dedb8df7f86ce71350e815ddaf6fe:

Not sure I am happy with the naming of this function. We are handling a blockindex here and we know implicitly that he are trying to catch errors here when the blockindex exists because there is just a header available but not the block. But someone looking at this function might think: "What does this have to do with a header if there is nothing about headers in the function?" It's not a blocker for me if others don't agree this is suboptimal but
...

💬 fjahr commented on pull request "rpc, rest: Improve block rpc error handling, check header before attempting to read block data.":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1742322249)
In 041b78ccede9974117dac5d8623fedec2f32471d:

nit: I think it would be cleaner and easier to read if you break this up.

```suggestion
const bool have_undo{is_not_pruned && WITH_LOCK(::cs_main, return blockindex.nStatus & BLOCK_HAVE_UNDO)};
if (have_undo) {
blockman.UndoReadFromDisk(blockUndo, blockindex);
}
```

(same in `rpc/rawtransaction.cpp`)