🤔 furszy reviewed a pull request: "Wallet: Add `maxfeerate` wallet startup option"
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-3174378537)
light review
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-3174378537)
light review
💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314492088)
In 677be6566b3b998a1d7bf66693c5d9748cf3190c:
It seems to me that we had certain relation between `max_fee` and `maxfeerate` before that we no longer have. What do you think about parsing `maxfeerate` first so that you could still check any relation between these two configurations? e.g. that `max_tx_fee` isn't ridiculously low/high compared to `maxfeerate`.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314492088)
In 677be6566b3b998a1d7bf66693c5d9748cf3190c:
It seems to me that we had certain relation between `max_fee` and `maxfeerate` before that we no longer have. What do you think about parsing `maxfeerate` first so that you could still check any relation between these two configurations? e.g. that `max_tx_fee` isn't ridiculously low/high compared to `maxfeerate`.
💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314493810)
In https://github.com/bitcoin/bitcoin/commit/677be6566b3b998a1d7bf66693c5d9748cf3190c:
nano nit: to not construct `CFeeRate` every time you need it, could extract
```c++
CFeeRate max_fee_rate(*max_tx_fee_rate);
// rest of the code
```
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314493810)
In https://github.com/bitcoin/bitcoin/commit/677be6566b3b998a1d7bf66693c5d9748cf3190c:
nano nit: to not construct `CFeeRate` every time you need it, could extract
```c++
CFeeRate max_fee_rate(*max_tx_fee_rate);
// rest of the code
```
💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314472896)
In 9768938066796:
It seems you should also change `getDefaultMaxTxFee` for `getMaxTxFee` too.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314472896)
In 9768938066796:
It seems you should also change `getDefaultMaxTxFee` for `getMaxTxFee` too.
💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314502479)
In https://github.com/bitcoin/bitcoin/commit/677be6566b3b998a1d7bf66693c5d9748cf3190c:
this change smells we are now allowing something that was previously not allowed. Like setting a fee through `settxfee` that is higher than `maxtxfee`.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314502479)
In https://github.com/bitcoin/bitcoin/commit/677be6566b3b998a1d7bf66693c5d9748cf3190c:
this change smells we are now allowing something that was previously not allowed. Like setting a fee through `settxfee` that is higher than `maxtxfee`.
💬 hebasto commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243161491)
> > we are not using ID-based translations
>
> @hebasto, are suggesting that the bitcoin-test clone where we tried it isn't representative? It did seem to me that we managed to reproduce and fix the instability by stabilizing the ids. But if we're not using the ids, why aren't the translations stable?
_We_ do not use them, but _Transifex_ does.
> What alternative do you suggest to stabilize them?
As I wrote in https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3241645029:
...
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243161491)
> > we are not using ID-based translations
>
> @hebasto, are suggesting that the bitcoin-test clone where we tried it isn't representative? It did seem to me that we managed to reproduce and fix the instability by stabilizing the ids. But if we're not using the ids, why aren't the translations stable?
_We_ do not use them, but _Transifex_ does.
> What alternative do you suggest to stabilize them?
As I wrote in https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3241645029:
...
💬 l0rinc commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243167834)
Do I understand you correctly that if we remove the IDs, Transifex will identify messages by content instead? Any reason we didn't do that before?
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243167834)
Do I understand you correctly that if we remove the IDs, Transifex will identify messages by content instead? Any reason we didn't do that before?
💬 hebasto commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243175770)
> Do I understand you correctly that if we remove the IDs, Transifex will identify messages by content instead?
That’s my guess, though I haven’t tested it.
> Any reason we didn't do that before?
I’m not aware of any specific reason.
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243175770)
> Do I understand you correctly that if we remove the IDs, Transifex will identify messages by content instead?
That’s my guess, though I haven’t tested it.
> Any reason we didn't do that before?
I’m not aware of any specific reason.
💬 l0rinc commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243190113)
My understanding is that **Translation Memory Fillups** feature is `Growth plan` feature only.
I have tried uploading the English translation without any ids (called keys in Transifex)
<img src="https://github.com/user-attachments/assets/5c3e560c-1ad5-4009-baf9-48ab13f0723b" />
adding back a single id results in only that value being imported:
<img src="https://github.com/user-attachments/assets/04dd3da9-9a5f-4176-ad50-685edd4593f7" />
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243190113)
My understanding is that **Translation Memory Fillups** feature is `Growth plan` feature only.
I have tried uploading the English translation without any ids (called keys in Transifex)
<img src="https://github.com/user-attachments/assets/5c3e560c-1ad5-4009-baf9-48ab13f0723b" />
adding back a single id results in only that value being imported:
<img src="https://github.com/user-attachments/assets/04dd3da9-9a5f-4176-ad50-685edd4593f7" />
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314529708)
Indeed but master is even more smelly because settxfee is not tx fee but fee rate.
See discussion https://github.com/bitcoin/bitcoin/pull/31278
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314529708)
Indeed but master is even more smelly because settxfee is not tx fee but fee rate.
See discussion https://github.com/bitcoin/bitcoin/pull/31278
👍 stickies-v approved a pull request: "build: Set AUTHOR_WARNING on warnings"
(https://github.com/bitcoin/bitcoin/pull/33144#pullrequestreview-3174447010)
ACK fa6497ba71e9573d341c1c051af09b3ec2fc8d74
It makes sense to have an option to have configuration warnings dealt as errors, and it's unfortunate that the most elegant way to do that is by somewhat abusing `AUTHOR_WARNING`, but this seems like the least bad solution to me, until CMake has a `-Werror=all` option.
---
I've also been thinking about an alternative approach. `configure_warnings` is currently used for:
1. missing Python interpreter, which is necessary (without build failure
...
(https://github.com/bitcoin/bitcoin/pull/33144#pullrequestreview-3174447010)
ACK fa6497ba71e9573d341c1c051af09b3ec2fc8d74
It makes sense to have an option to have configuration warnings dealt as errors, and it's unfortunate that the most elegant way to do that is by somewhat abusing `AUTHOR_WARNING`, but this seems like the least bad solution to me, until CMake has a `-Werror=all` option.
---
I've also been thinking about an alternative approach. `configure_warnings` is currently used for:
1. missing Python interpreter, which is necessary (without build failure
...
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314535030)
Hmm, these are different options that can be set for various use cases see comment
https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915276592 and https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919611531
Not exactly sure which relation do you want a check for because there is already check if bounds for each individual option.
Perhaps suggest code ?
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314535030)
Hmm, these are different options that can be set for various use cases see comment
https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1915276592 and https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919611531
Not exactly sure which relation do you want a check for because there is already check if bounds for each individual option.
Perhaps suggest code ?
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314544616)
Yeah, done.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314544616)
Yeah, done.
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314544789)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314544789)
Thanks, fixed.
💬 furszy commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314549905)
> Are you suggesting that I add a test case that checks that we dont create transaction when the tx fee as a result of the fee rate provided by settxfee is more than maxtxfee?
I’d be surprised if we don’t already have a test for that.
> Note: This wallet RPC is likely going to be removed soon.
"soon" is very relative here.
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2314549905)
> Are you suggesting that I add a test case that checks that we dont create transaction when the tx fee as a result of the fee rate provided by settxfee is more than maxtxfee?
I’d be surprised if we don’t already have a test for that.
> Note: This wallet RPC is likely going to be removed soon.
"soon" is very relative here.
🤔 l0rinc reviewed a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3174344920)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3174344920)
Concept ACK
💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314454304)
not sure I understand why this was needed
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314454304)
not sure I understand why this was needed
💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314448739)
could we fix this in https://github.com/bitcoin-core/leveldb-subtree/pulls instead?
I have opened https://github.com/bitcoin-core/leveldb-subtree/pull/56 (I'm also fine with doing it in a different PR, just wanted to give the possibility to solve it in a cleaner way)
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2314448739)
could we fix this in https://github.com/bitcoin-core/leveldb-subtree/pulls instead?
I have opened https://github.com/bitcoin-core/leveldb-subtree/pull/56 (I'm also fine with doing it in a different PR, just wanted to give the possibility to solve it in a cleaner way)
💬 l0rinc commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2314579166)
I didn't mean my response to be offensive before, and while still not sure I fully follow the arguments, I agree that stabilizing the formatting and unifying class/struct braces are separate concerns, so I have removed the unification, only kept the change making formatters explicit.
(https://github.com/bitcoin/bitcoin/pull/32813#discussion_r2314579166)
I didn't mean my response to be offensive before, and while still not sure I fully follow the arguments, I agree that stabilizing the formatting and unifying class/struct braces are separate concerns, so I have removed the unification, only kept the change making formatters explicit.
💬 l0rinc commented on pull request "clang-format: make formatting deterministic for different formatter versions":
(https://github.com/bitcoin/bitcoin/pull/32813#issuecomment-3243270669)
Kept the formatter config only without the class/struct brace unification, it's ready for re-review, thanks for the comments.
(https://github.com/bitcoin/bitcoin/pull/32813#issuecomment-3243270669)
Kept the formatter config only without the class/struct brace unification, it's ready for re-review, thanks for the comments.