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

💬 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_r1742280583)
In 856c162fb6cdbb5715cae23d1b690d5bb3812afc: The function is introduced just two commits earlier, I think the `static` can be dropped there and the header entry added there as well.

💬 hebasto commented on issue "cmake: incorrect assumption that `Debug` build type will not use optimisations":
(https://github.com/bitcoin/bitcoin/issues/30800#issuecomment-2326899886)
> 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.

I think we should first prioritize coordinating the optimization and debugging flags between the depends subsystem and the main build system:
1. Should the optimization and debugging flags coincide, or are they allowed to differ?
2. If the optimization and debugging can differ between the depends subsystem and in the main b
...

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742328675)
`BUILD_FOR_FUZZING` has nothing to do with fuzz engines. According to the documentation it is an option to "Build for fuzzing. Enabling this will disable all other targets and override BUILD_FUZZ_BINARY."

Can you explain this a bit better? Maybe with an example output of a compile error or otherwise steps to reproduce?

💬 hebasto commented on issue "cmake: Installed static kernel library is unusable":
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2326907165)
> 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.

+1

💬 TheCharlatan commented on issue "cmake: Installed static kernel library is unusable":
(https://github.com/bitcoin/bitcoin/issues/30801#issuecomment-2326911621)
> Instead of target_link_libraries(bitcoinkernel bitcoin_crypto) do add_library(bitcoinkernel $<TARGET_OBJECTS:bitcoin_crypto>.

He, I think I was reading the same stack overflow post. I think this doesn't quite work, because those libraries have sub-dependencies that don't get included easily. So we'd need more logic to get the correct crc32 and crypto extensions if we attempt that patch. I'm not sure that would be really desirable, since we'd be duplicating a bunch of implementation selectio
...

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742339471)
Thanks for the reference. Looks like it was added in commit dc1e7ad7a5713d885f70ccc6c93e7a4c07e76559 by @ryanofsky. My preference would be to just drop the prefix "Stable, backwards-compatible", because it is unclear whether it refers to the build system artefacts, or the consensus rules. In either case, the prefix seems either wrong or an approximation that lacks nuance.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1742340130)
what would happen when we restart with a different data directory, it will have to refetch everything, right? Or if we had a reorg on the very first block maybe?
Maybe @sipa can help us out here...

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742342524)
Thanks. Makes sense. I guess the comment should be adjusted to say that this won't be possible to do any time soon, or at all, for the reason given by you?

📝 tdb3 reopened a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793)
This PR adds a new rpc, `getorphantxs`, that provides the caller with a list of orphan transactions.

Currently looking to gauge interest on the concept, or any other productive feedback. If there is little/no interest, will close the PR for history.

PR https://github.com/bitcoin/bitcoin/pull/30784 adds a test that excessively large transactions aren't added to the orphanage. It checks the debug.log for confirmation that the transaction was not added.

As discussed in comment https://gi
...

💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1742348150)
Thanks for testing @fanquake. Just to clarify: I left the review comment, because running the tests was added as part of this pull request, even though it wasn't present previously with autotools.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742352471)
Right. Initially, the thought was to break clean from PeerManager/TxOrphanage, but CTransactionRef is a shared_ptr so could access the CTransaction even if it was dropped in TxOrphanage.

🤔 mzumsande reviewed a pull request: "validation: do not wipe utxo cache for stats/scans/snapshots"
(https://github.com/bitcoin/bitcoin/pull/30610#pullrequestreview-2277942711)
Concept ACK

The test `interface_usdt_utxocache.py` fails because `generate()` in `wallet.py` calls `scantxoutset`, which doesn't wipe the cache anymore. As a result, the number of utxos flushed to disk in later flushes changes.

Re: "currently-unused CreateUTXOSnapshot" in the PR description - it's used to create snapshots (`dumptxoutset`).