💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2507786099)
Tested 346218ae614a90afdf4e20621cfa66e40700116a on:

* Intel macOS 13.7.1 with depends as well as `qt` 6.7.3 from homebrew
* Apple Silicon macOS 15.1.1 with depends as well as `qt` 6.7.3 from homebrew
* Ubuntu 24.10 with Wayland, with depends as well as `qt-wayland`
* A guix build tested on Windows 11
* Guix build tested on the first three machines

> Starting with Qt 6.5.0, the libxcb-cursor0 package is required to be installed at runtime.

There's no mention of this in `build-unix.md
...

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1863360925)
758c41a303a2471c7f92bf1f68f4582c146f26f5: is there anything other than QT in our guix build that will magically start using ninja once we add it here?

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1863519065)
I don't think setting `coinbase_max_additional_weight` to `0` is the right approach.

I'll comment on #21950 first.

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1863522185)
I don't have an opinion on whether `currentblockweight` should include the reservation. But the documentation should be clarified either way.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1863532743)
I guess no. See https://github.com/bitcoin/bitcoin/pull/31171.

🚀 glozow merged a pull request: "doc, test: more ephemeral dust follow-ups"
(https://github.com/bitcoin/bitcoin/pull/31371)

🤔 hodlinator reviewed a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2468800236)
Code reviewed 9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24

Good that `AddFlags()` was made `static` so one doesn't have both `this` and `self` any longer! 😅

Surprised that `CCoinsCacheEntry::m_next` and `m_prev` were not nulled out before this PR as is done in `SetClean()` in 9d61b956d91153d25d06047de03133bc2e23b2a6, but seems reasonable. Only caller of `SetClean()` outside of tests is `CoinsViewCacheCursor::NextAndMaybeErase()`. That's the only commit where I feel that more domain expertise
...

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862704385)
nit: Could add comment
```C++
// Check that calling `SetClean` a second time has no effect
```

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862714606)
nit: Randomly gets curlies in c4eb31be3e56936479921b7556fcba1747867d02 but could have been there already in 7b3cfcf601524e442d71afbe448a526908e37e99.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862729959)
Why is the order between cache & modify parameters changed? Makes `ccoins_add`-diff harder to review.
```suggestion
static void CheckAddCoin(CAmount base_value, const MaybeCoin& cache_coin, CAmount modify_value, const CoinOrError expected, bool coinbase)
```

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862743627)
nit: Why not use `std::nullopt` directly instead of keeping the `MISSING`-alias when switching to `optional`?
I tried, and had to use like `MaybeCoin{}` in 3 places where it was used in combination with other such values... so I'll give you that it's not entirely straight-forward.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862752926)
nit: Less verbose (`MaybeCoin` type should be enough)?
```suggestion
constexpr MaybeCoin SPENT_DIRTY {{SPENT, CoinEntry::State::DIRTY}};
```

Also: Needless noise going from ` = `-initialization in early commits to brace-initialization in later ones, could be braces from the start.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862708127)
New TODO?

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1863509899)
Incomplete refactor:
```suggestion
if (dirty) CCoinsCacheEntry::SetDirty(*it, sentinel);
if (fresh) CCoinsCacheEntry::SetFresh(*it, sentinel);
```

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862754511)
`CoinEntry` could be `constexpr`-friendly from the very start instead of gaining that ability towards the end, meaning `SPENT_DIRTY` & co could be `constexpr` from the start, reducing noise.

Could also place `struct CoinEntry` below the old `DIRTY`/`FRESH` constants in order to decrease size of diffs.

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1862760164)
(nit: Introduced 2 commits before they're used, and `<string>` is only used transiently, where constants could be using `auto` (implicit `char[]`) from the start).

💬 Sjors commented on issue "Duplicate coinbase transaction space reservation in CreateNewBlock":
(https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2507864614)
TIL about this, despite having authored #30356.

I think `-blockmaxweight` should refer to the actual worst case block, i.e. assuming the pool uses all 4000 WU. So its default should be equal to `MAX_BLOCK_WEIGHT`.

We do need to make sure that if a miner has been setting `-blockmaxweight=4000000` manually, nothing breaks.

An additional argument for having `-blockmaxweight` not adjust for the reservation, is that in Stratum v2 each connected Template Provider (i.e. a pool or an individual
...

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2507869514)
I'm in favor of fixing this bug, but differently, see https://github.com/bitcoin/bitcoin/issues/21950#issuecomment-2507864614

👍 TheCharlatan approved a pull request: "cmake, qt: Use absolute paths for includes in MOC-generated files"
(https://github.com/bitcoin/bitcoin/pull/31361#pullrequestreview-2469994097)
ACK 6f4128e3a838d03f46d397c15bc5333287e14863

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1863578230)
yes, see the mentioned derisked PR: https://github.com/bitcoin/bitcoin/pull/30673/files#diff-1ef3b6a1936b50f3d5ec4a1786d9e2d63d1a3e1815b103e67f20601995f355b4R152-R154