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

💬 willcl-ark commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2733025378)
Concept ACK

I forgot about this issue then bumped into it in docker again today trying to add the ability to cross-compile to arm64 to my Dockerfile:

<details>
<summary>Details</summary>

```log
#14 92.71 /usr/bin/ld: /src/bitcoin/depends/aarch64-linux-gnu/lib/libevent_core.a(buffer.c.o): Relocations in generic ELF (EM: 183)
#14 92.71 /usr/bin/ld: /src/bitcoin/depends/aarch64-linux-gnu/lib/libevent_core.a(buffer.c.o): Relocations in generic ELF (EM: 183)
#14 92.71 /usr/bin/ld: /src/b
...

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

To be fair, being able to test under Wine can be useful, if you're building/developing/testing something for windows but don't have a native windows machine available immediately. Sure it's possible to e.g. create windows server VMs at amazon EC but the extra cost and hassle is only warranted for final testing. But also agree that Wine support is not something that's worth putting a lot of effort in maintaining. But also wouldn't want to br
...

🚀 ryanofsky merged a pull request: "descriptor: check whitespace in keys within fragments"
(https://github.com/bitcoin/bitcoin/pull/31603)

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2733092556)
@DrahtBot I've rebased and force-pushed 2 hours ago, but the update is yet to reflect here see https://github.com/bitcoin/bitcoin/compare/master...ismaelsadeeq:bitcoin:05-2023-ignore-transactions-with-parents

📝 Chand-ra opened a pull request: "test: replace assert with assert_equal and assert_greater_than"
(https://github.com/bitcoin/bitcoin/pull/32091)
In `test/functional/interface_usdt_net.py`, `assert_equal` is already used to check for equality between objects. Replace `assert.*==` with `assert_equal` and `assert.*>` with `assert_greater_than` to further easify debugging.

💬 maflcko commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2733111126)
This is a GitHub bug, but they are not going to fix it. The only way to fix it is by doing another rebase (or for a GitHub staff to manually clean it). See https://github.com/maflcko/DrahtBot/issues/43#issuecomment-2653314333

💬 maflcko commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#issuecomment-2733115158)
lgtm ACK 93fdaf1ca00de0bc46eb2455732582c34cef58b5

💬 maflcko commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2000980971)
actually, this looks wrong. Let's hope the test/CI fails.