💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863319077)
I find these kind of messages to be more readable than `Config file: %s (is directory, not file)` or `"Config file: <disabled>`
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863319077)
I find these kind of messages to be more readable than `Config file: %s (is directory, not file)` or `"Config file: <disabled>`
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863337310)
My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863337310)
My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863326293)
nit: "should never happen" -> but it just did, so can we either provide more context or remove the statement that was just invalidated?
```suggestion
assert len(debug_logs) == 1, (
'Found multiple debug.log files, expected only one:\n\t' +
'\n\t'.join(str(f) for f in debug_logs)
)
```
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863326293)
nit: "should never happen" -> but it just did, so can we either provide more context or remove the statement that was just invalidated?
```suggestion
assert len(debug_logs) == 1, (
'Found multiple debug.log files, expected only one:\n\t' +
'\n\t'.join(str(f) for f in debug_logs)
)
```
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863338147)
Does it signal the lack of `self.stop_node(0)`? Does this mean the test run order matters now?
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863338147)
Does it signal the lack of `self.stop_node(0)`? Does this mean the test run order matters now?
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863323186)
same nit: `\n` is likely redundant in these cases as well
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863323186)
same nit: `\n` is likely redundant in these cases as well
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863364323)
it's hard to review 14 commits, as hinted before, could the fix be in the same commit as the test - I personally am lacking a lot of context and wouldn't want this commit to be merged without making sure a test is added as well
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863364323)
it's hard to review 14 commits, as hinted before, could the fix be in the same commit as the test - I personally am lacking a lot of context and wouldn't want this commit to be merged without making sure a test is added as well
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863392073)
I'm fine with doing it in a different PR - will leave it up to you
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863392073)
I'm fine with doing it in a different PR - will leave it up to you
💬 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.
(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.
(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
...
(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?
(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.
(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.
(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.
(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)
(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
...
(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
```
(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.
(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)
```
(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.
(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.