💬 fanquake commented on pull request "depends: build libevent with `-D_GNU_SOURCE`":
(https://github.com/bitcoin/bitcoin/pull/30743#issuecomment-2326027285)
For now I've opted with matching the prior Autotools behaviour, and always building with `-D_GNU_SOURCE`, which is also easy to backport to `28.x`.

💬 mzumsande 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#issuecomment-2326029369)
> @andrewtoth the CI failure in the test-each-commit job is unrelated to the changes (missing zmq package, fixed in #30749).

Thanks, I also wasn't sure! I've just rebased again.

💬 maflcko commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#issuecomment-2326034719)
review-only ACK 787dfaf084a3952319778da9cbcda9d7d619e4ee

Only change since last review is linking with libbitcoin_util.

It would be nice to better understand this change, but this can be done later and is not a blocker to for a test-only change.

Also, the depends behavior can be documented better, or be changed, but this is probably best done in a separate issue or pull request, so that this test-only change stays focussed.

👍 fanquake approved a pull request: "doc: update documentation and scripts related to build directories"
(https://github.com/bitcoin/bitcoin/pull/30741#pullrequestreview-2276903597)
ACK 6a68343ffbf3291eb243d90c00df50e672ff3944 - we still need to followup with other scripts/devtools, and likely unify what we are doing in some way, but this is an improvement.

🚀 fanquake merged a pull request: "doc: update documentation and scripts related to build directories"
(https://github.com/bitcoin/bitcoin/pull/30741)

🤔 l0rinc reviewed a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611#pullrequestreview-2276771072)
Thanks for quick responses, my main concern at this stage is that the commit messages don't explain the changes in enough detail and some symbols might need a rename after the change

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741658439)
we don't have `m_last_write` anymore

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741663448)
nit: 1 hour is a *duration* or *interval* or *period*, but technically not a *time*:
```suggestion
// The periodic flush interval is 1 hour
```

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741667937)
could you please explain the `flush` removal in the corresponding commit message as well?

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

nit: just an observation, the starting `!` hides well here, if you think it read better, here's an alternative (though I see that IsNull is used in a few other places):
```suggestion
if (CoinsTip().GetBestBlock() != uint256::ZERO) {
```

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741685428)
is the name still accurate, now that a periodic write can also trigger it (i.e. not just flushes, like before)?

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741737264)
I think the convention is:
```suggestion
// Copyright (c) 2024-present The Bitcoin Core developers
```

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741725078)
I know this line didn't change here, I'm looking for answers:
I find it quite confusing that most of the side-effects of this method are actually done inside if conditions - I wanted to understand whether the name `fDoFullFlush` is accurate, but had a hard time understanding which methods are just checking state (e.g. `CheckDiskSpace`, I assume) and which ones are changing state (e.g. `empty_cache ? !CoinsTip().Flush() : !CoinsTip().Sync()`).
So my question is whether it's fair to call the con
...

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1741741467)
we do more than `add randomness to periodic write interval` in this commit now, could you please split it up a bit more?

💬 ismaelsadeeq commented on pull request "Fee Estimation via Fee rate Forecasters":
(https://github.com/bitcoin/bitcoin/pull/30157#issuecomment-2326075219)
Rebased and added newly introduced files to cmakelist file instead makefile

📝 l0rinc converted_to_draft a pull request: "refactor: Extend CScript with `operator<<` for `std::array`"
(https://github.com/bitcoin/bitcoin/pull/30765)
Split out of https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722326803.

Extracted the existing `CScript` vector serialization into two private methods (for size and data handling), reducing duplication between `vector` and `array` methods (relying only on their size, begin, and end methods).

This change enables the use of `CScript() << "..."_hex_u8`, skipping the `vector` conversion before serializing to the prevector in `CScript`.

Note that after this change, it would techni
...

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2326119633)
Rebased and added newly introduced files to cmakelist file instead of makefile

💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#issuecomment-2326144002)
> > Sets the "Debug" build configuration when depends are built with DEBUG=1, ensuring that linux_debug_CPPFLAGS from depends are passed to the main build system.

> Is this not a bug?

No, it is not. Rather it is a design feature.

> If depends is built (in any configuration/with any options/flags) then the expectation would be that everything is passed to CMake (regardless of the CMake build type).

While both depends and CMake have configuration-specific flags, it seems unreasonable t
...

📝 0xB10C converted_to_draft a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593)
Currently, if the tracepoints are compiled (e.g. in depends and release builds), we always prepare the tracepoint arguments regardless of the tracepoints being used or not. We made sure that the argument preparation is as cheap as possible, but we can almost completely eliminate any overhead for users not interested in the tracepoints (the vast majority), by gating the tracepoint argument preparation with an `if(something is attached to this tracepoint)`. To achieve this, we use the optional sem
...

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2326162347)
Rebased this on autotools removal, but the new way of detecting if the systemtap headers are present doesn't seem to work (I didn't do `cmake -B build -DWITH_USDT=ON` locally). Marking this as draft until I figure out how it works and rebase this on https://github.com/bitcoin/bitcoin/pull/30741