💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2314389493)
I’m not exactly sure how this works; my IDEs break here. Is my understanding correct that we’re just piggybacking a new value onto a `Future`’s per-instance `__dict__`?
Would it be cleaner to use a Future-to-string dictionary instead?
```python
self.job_name = {}
...
future = self.executor.submit(proc_wait, task)
self.job_name[future] = test
self.jobs.append(future)
...
print("Remaining jobs: [{}]".format(", ".join(self.job_name[f] for f in self.jobs)))
```
(I checked, and it seems di
...

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2314417456)
is it still correct to claim that we're sleeping?

👍 l0rinc approved a pull request: "test: Remove polling loop from test_runner (take 2)"
(https://github.com/bitcoin/bitcoin/pull/33141#pullrequestreview-3174274620)
tested ACK fa7a3b5fb04465a3fbadf1ca62eac26827c1ef1d

there are still a few things I'd prefer before we merge, but won't block

💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#discussion_r2314392659)
nit: since this is new code, maybe we could decompose `done` and `not_done` here already
```python
done, not_done = futures.wait(self.jobs, timeout=.5, return_when=futures.FIRST_COMPLETED)
self.jobs = list(not_done)
ret = []
for fut in done:
```

💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-3243043898)
Of course, paused it since I'm not exactly sure how do handle single-byte entries: they measurably speed up the scenarios, but the resulting code is quite ugly. Do you have a suggestion? Should I attempt that in a separate PR and ignore it here?

💬 l0rinc commented on pull request "coins,refactor: Reduce `getblockstats` RPC UTXO overhead estimation":
(https://github.com/bitcoin/bitcoin/pull/31449#discussion_r2314439240)
yes, both are over-estimating, but this is closer to the actual functioning, the previous one explicitly adds the bool which isn't separate.
We can also just fix it by not making this a `constexpr` but a function of the actual values, but I'd argue the current versions is still better.

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