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

💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742355241)
Sure. The logic is follows:
1. The `MSAN_FLAGS` variable's value is `... -g -O1 ...`.
2. This value is assigned to `CXXFLAGS` for building depends and become the value of `CMAKE_CXX_FLAGS` in the toolchain file.
3. Being configured with `-DCMAKE_BUILD_TYPE=Debug`, the build system may assign `-O0 -ftrapv -g3` to `CMAKE_CXX_FLAGS_DEBUG`.
4. When invoking a compier, `CMAKE_CXX_FLAGS` and `CMAKE_CXX_FLAGS_DEBUG` are concatenated, resulting in `.. -g -O1 ... -O0 ... -g3 ...`. This means that fla
...

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742358615)
Good suggestion. Updated to an array of objects, with each object containing txid and wtixd. Some fields that would think make sense to add:

- Expiration time
- Size info
- Fee info
- Originator (what sort of info would we like to see here? address?)

Alternatively, maybe could replace the wrapping array with an object, with each orphan being keyed by its wtixd?

💬 maflcko commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742358892)
Maybe this can be marked Experimental, so that the interface can easily be changed, given that the goal would mostly only be for testing/debugging? Also, the RPC could be hidden, like other similar RPC, for example `estimaterawfee`?

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1742363742)
Thank you. Both of those suggestions are reasonable to me. Will update soon. Do you have a preference?

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#issuecomment-2326985542)
There are now 3 ACKs so unless need to retouch I will make a quick follow-up addressing the open comments from @pablomartin4btc and @mzumsande .