💬 marcofleon commented on pull request "fuzz: Use immediate task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2732695727)
> Happy to include that here

No need, just confirming for my own understanding.

🤔 musaHaruna reviewed a pull request: "test: Check datadir cleanup after assumeutxo was successful"
(https://github.com/bitcoin/bitcoin/pull/32033#pullrequestreview-2694039878)
Post ACK [52482cb](https://github.com/bitcoin/bitcoin/pull/32033/commits/52482cb24400f8c44ba9628aaaecb7c04b11beb2)

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2000800309)
fixed

💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#issuecomment-2732777108)
@laanwj I clarified the PR description by adding "This PR does not change the non-depends build."

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732820174)
> > Both src/minisketch and src/secp256k1 are subtrees. Therefore, they are out of this PR scope.
>
> I don't think they are entirely out of scope? Currently those tests (as well as univalue) are wrapped for (previously in the CI, and still possible using `RUN_UNIT_TESTS`) running under Wine, but you're removing all of that, and the rest of the Wine infra entirely here (no-longer running those tests), or providing a way for anyone to run them locally?

Wine is not a supported platform for B
...

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732825021)
Please let me know if I have left anyone's comment unaddressed.

💬 vasild commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2732857665)
A few notes:

* Bitcoin Core would try to maintain up to 12 outbound connections to peers from all networks that are reachable (IPv4, IPv6, Tor, I2P, CJDNS). From these usually just a few are I2P. So, it is reasonable to assume that from 2,3,4 to 12 I2P connections will be needed. But the case with 12 is when I2P is the only reachable network which is not advisable because of the increased Sybil likelihood.

* This PR is not going to create a storm of I2P sessions creation at startup because
...

💬 fanquake commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732864237)
> Wine is not a supported platform for Bitcoin Core.

If that is the case, then we should stop maintaining workarounds for Win in this codebase. i.e: https://github.com/bitcoin/bitcoin/blob/d61a847af0b4363c228c432556eeadefa5616b3a/test/util/test_runner.py#L161-L163

💬 polespinasa commented on issue "wallet: rpc: `settxfee` sets the wallet feerate not fee":
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2732868717)
> If the goal is to only support one unit, it would be better to adjust all places to return that unit, so that it can be used in round-trips. Otherwise, users will have to convert values they receive from the RPC to pass them to another RPC of the same program.

I agree, @ismaelsadeeq maybe we could move this issue to a more "general" one where we migrate all units from BTC/kvB to sat/vB?

🤔 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-2620965099)
After our last discussion (and with the new AssumeUTXO included) I've rebased and re-reviewed and added examples to my suggestions to simplify the review.

My remaining concerns are that AssumeUTXO doesn't warn in the same way as other dumps do, a few leftover refactoring misses, redundant calculation now that we have a dirty count, missing code coverage for modified code, a leftover memory accounting bug in affected area, and a few code nits to make the change less surprising.

```
diff --
...

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1958176752)
nit: parameters slightly unaligned with first parameter

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2000724657)
in other cases we set it to dirty first and only increment `m_dirty_count` after (shouldn't matter on single thread but at least we don't have to think about whether that's the case or not):
```suggestion
CCoinsCacheEntry::SetDirty(*itUs, m_sentinel);
++m_dirty_count;
```

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2000703255)
[Leftover](https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925499202) `num_dirty`

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2000583711)
[leftover](https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925499202) `num_dirty` (& other comments here end with fullstop)
```suggestion
// Recompute m_dirty_count.
````

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2000616997)
To clarify, currently AssumeUTXO doesn't display the warning:
```bash
2025-03-18T10:08:47Z [snapshot] 184000000 coins loaded (99.56%, 29332 MB)
2025-03-18T10:08:48Z [snapshot] loaded 184821030 (29456 MB) coins from snapshot 000000000000000000010b17283c3c400507969a9c2afd1dcf2082ec5cca2880
2025-03-18T10:08:48Z FlushSnapshotToDisk: saving snapshot chainstate (29456 MB) started
2025-03-18T10:09:57Z FlushSnapshotToDisk: completed (69461.66ms)
```

But if we moved the warning introduced in htt
...

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1958190950)
Do we still need to track [`CCoinsViewDB::BatchWrite#changed`](https://github.com/bitcoin/bitcoin/blob/master/src/txdb.cpp#L96) now that we're tracking dirty count already?
I think we should be able to use the new `m_dirty(_count)` [there](https://github.com/bitcoin/bitcoin/blob/4feaa28728442b0fd29a677d2b170a05fdf967c0/src/txdb.cpp#L150) instead, i.e. something like:
```C++
size_t dirty_count{cursor.GetDirtyCount()};
size_t changed{0};
...
if (dirty_count > WARN_FLUSH_COINS_COUNT) LogWarni
...

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2000688081)
Following @andrewtoth's solution we can do:
```C++
void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
const auto mem_usage{coin.DynamicMemoryUsage()};
auto [it, inserted] = cacheCoins.try_emplace(std::move(outpoint), std::move(coin));
if (inserted) {
cachedCoinsUsage += mem_usage;
CCoinsCacheEntry::SetDirty(*it, m_sentinel);
++m_dirty_count;
}
}
```

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2000712052)
I don't mind if we're mixing booleans and ints as long as the types and names are obvious, e.g.
```C++
size_t dirty = cache_coin ? cache_coin->IsDirty() : 0;
auto cursor{CoinsViewCacheCursor(usage, dirty, sentinel, map, /*will_erase=*/true)};
```
where we're setting a parameter called `dirty` to the result of `IsDirty`, but it gets converted to dirty count.
We could obviate this by calling it `dirty_count` here as well:
```C++
CoinsViewCacheCursor(size_t& usage LIFETIMEBOUND,

...

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2000696000)
I will do this later to test reorg behavior in a functional test, you can resolve this comment.

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2732978135)
> > Wine is not a supported platform for Bitcoin Core.
>
> If that is the case, then we should stop maintaining workarounds for Wine in this codebase. i.e:
>
> https://github.com/bitcoin/bitcoin/blob/d61a847af0b4363c228c432556eeadefa5616b3a/test/util/test_runner.py#L161-L163

Sure. Leaving it for follow ups.