💬 l0rinc commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3243067816)
> 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? What alternative do you suggest to stabilize them?

> shasum of the full content can be used as an id for the translation string

@maflcko we could of course do that, but it would likely invalidat
...

💬 ismaelsadeeq commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3243084444)
https://cirrus-ci.com/task/6512217929023488?logs=ci#L3103

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-3243087856)
C.I Failure is not related see https://github.com/bitcoin/bitcoin/issues/33244

💬 l0rinc commented on pull request "build: add `-Wleading-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3243095040)
ACK b271b342e36000952a389c05b409bab691a4e4d2

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-3243102978)
Done.

💬 ismaelsadeeq commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#issuecomment-3243132085)
Rebased due to merge conflict.

🤔 furszy reviewed a pull request: "Wallet: Add `maxfeerate` wallet startup option"
(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`.

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

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

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

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


...

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

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

💬 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" />

💬 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

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

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

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(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.