💬 maflcko commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1863408856)
Parsing it twice and casting to double in-between seems highly fragile. It would be better to directly parse into a fraction.

💬 maflcko commented on issue "wallet: rpc: `settxfee` sets the wallet feerate not fee":
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2507658805)
On a greater scope, I wonder if it is a good approach to one-by-one deprecate one unit and add another.

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.

💬 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