📝 fanquake locked a pull request: "doing whats needed."
(https://github.com/bitcoin/bitcoin/pull/31776)
Cant Automatically merge.<!-
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements
...

💬 pinheadmz commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938258005)
Then perhaps something like "Reserve space for the fixed-size block header plus the largest coinbase transaction the mining software may add to the block"

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2628948969)
Updated fb6e0deee18dff38219c9646d2461a40de03ed66 -> f926d7ef34773b7836e94491a16021288c125b11 ([kernelApi_20](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_20) -> [kernelApi_21](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_21), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_20..kernelApi_21))

* Added symbol exports where appropriate in the header to enable windows support
* Completely replaced the existing bitcoin-chainstate with the new kernel-API-only
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1938274204)
Ok, added symbol exporting now, so we should have somewhat working windows support.

💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938277339)
And maybe

```suggestion
leading to a minimum template size of `3,992,000` in any case.
```

💬 fjahr commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938278047)
And maybe add something like "Be careful about lowing -blockreservedweight to 4,000 and ensure that your coinbase transaction does indeed not exceed 4,000 since there were previously always 8,000 WU available".

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938267534)
👍 10m seems like a good approximation for the previous value:

> 2025-01-31T09:45:42Z UpdateTip: new best=0000000000000000cc22cc938f92ecc3a3844f23775c4ad4e66404563c8594c8 height=287000 version=0x00000002 log2_work=76.752105 tx=33340892 date='2014-02-2
1T05:18:42Z' progress=0.028794 cache=1245.8MiB(10004443txo)

🤔 l0rinc requested changes to a pull request: "Use number of dirty cache entries in flush warnings/logs"
(https://github.com/bitcoin/bitcoin/pull/31703#pullrequestreview-2588328910)
I have reviewed it in multiple steps, I have a few remaining questions and concerns, let me know if I can help in any other way.

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938266129)
This now hides `GetDirtyCount` in `CCoinsViewCache` with a `size_t` return value (changed to `size_t&` here), used later as `cache.usage() +=` and `cache.GetDirtyCount() +=`.

To avoid the confusing override and the method return value assignment, please consider making them explicit setters/adders:

```C++
void AddUsage(size_t usage) const { cachedCoinsUsage += usage; }
void AddDirtyCount(size_t count) const { m_dirty_count += count; }
```

```C++
cache.AddUsage(InsertCoinsMapEntry(ca
...

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938275296)
The documentation of `EmplaceCoinInternalDANGER` claims that:
> Emplace a coin into cacheCoins without performing any checks, marking the emplaced coin as dirty.

How come we're updating `cachedCoinsUsage` unconditionally in this method - but `m_dirty_count` only when inserted?

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938269653)
This change doesn't seem to be covered by `coins_tests` at all.
Can we extend `SimulationTest` to call `stack.back()->EmplaceCoinInternalDANGER` instead of `AddCoin` randomly?

Maybe something like:
```C++
if (COutPoint op(txid, 0); !stack.back()->map().contains(op) && !(coin.IsSpent() || (m_rng.rand32() & 1))) {
stack.back()->EmplaceCoinInternalDANGER(std::move(op), std::move(newcoin));
} else {
stack.back()->AddCoin(std::move(op), std::move(newcoin), /*possible_overwrite=*/(!co
...

💬 andrewtoth commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938281310)
That's a bug from #28280. It should be done conditionally. I fix it in the first commit of #31132. The only caller of this method will not add coins that already exist in the cache though, so it will always enter the `if (inserted)` block.

👍 l0rinc approved a pull request: "build: simplify by flattening the dependency graph"
(https://github.com/bitcoin/bitcoin/pull/30911#pullrequestreview-2588345575)
utACK 12fa9511b5fba18e83f88b7b831906595bcf2116

Compared to previous version, `CMAKE_OPTIMIZE_DEPENDENCIES` was moved after [project](https://github.com/bitcoin/bitcoin/blob/master/CMakeLists.txt#L43-L48) and only assigned if user didn't provide any value.

```
git diff HEAD a6efc4ca368661fb82136ea4d68f3043d058dc48
diff --git a/CMakeLists.txt b/CMakeLists.txt
index edfb63926a..86d5ed6448 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -40,6 +40,11 @@ set(CMAKE_TRY_COMPILE_PLATFORM
...

💬 l0rinc commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1938283672)
I'm not sure I understand when we're using ON/OFF and when TRUE/FALSE, but I understood both are accepted, so it's just a nit

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938284772)
Thanks for validating it, I only noticed it now, when trying to test `EmplaceCoinInternalDANGER` in [SimulationTest](https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938269653)

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1938285967)
Nice! Just to be clear: I only had issues with compiling the bitcoin-chainstate target, the bitcoinkernel library already was working fine (at least the functions covered with py-bitcoinkernel's test suite, which is not yet 100%) with the mingw32 cross-compiled binary.

(But I suspect you're talking about "somewhat working **native** windows support", which I'm not using in my pipelines)

💬 sipa commented on pull request "test: Fuzz the human-readable part of bech32 as well":
(https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2628986852)
ACK 9b7023d31a3ec95f66b45f0ecb47e79762d74854

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938289597)
coinbase transaction, the block header, etc?

💬 sipa commented on pull request "optimization: precalculate SipHash constant XOR with k0 and k1 in SaltedOutpointHasher":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-2628994530)
I'm not entirely convinced about the approach here: it's leaking a SipHash implementation detail (the way the initial v0..v3 values are computed) into a higher layer (util/hasher.h).

I would suggest instead to have a `Uint256ExtraSipHasher` class instead of the `SipHashUint256Extra` function, with a constructor that takes k0, k1 like the function current before, and computes the v0..v3 values, and then an `uint64_t Uint256ExtraSipHasher::operator()(const uint256& val, uint32_t extra) noexcept
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2628997388)
Rebased f926d7ef34773b7836e94491a16021288c125b11 -> 817865d57daa822370b0f67e1e079fdd25ab3130 ([kernelApi_21](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_21) -> [kernelApi_22](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_22), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_21..kernelApi_22))

* Fixed conflict with #30965, which necessitated some changes to the API: The block manager options are now responsible for the options affecting the block tree db.
...

👍 i-am-yuvi approved a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758#pullrequestreview-2588373657)
re-ACK 2b94e1abbfdef058546221d63f53b0b58eea22ea