💬 maflcko commented on pull request "coins,refactor: Reduce `getblockstats` RPC UTXO overhead estimation":
(https://github.com/bitcoin/bitcoin/pull/31449#discussion_r2313188865)
isn't the serialized format using varint compression anyway, so any hardcoded overhead will be wrong here anyway?

I'd say it is fine to leave this as-is, because both this pull and current master are equally "wrong" and in the presence of doubt, it seems better to not change the code?

💬 Sjors commented on pull request "test: add logging to mock external signers":
(https://github.com/bitcoin/bitcoin/pull/32928#issuecomment-3241304440)
@maflcko updated the PR description. I assume more logging is still useful though.

💬 Sjors commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3241320504)
Now that the subtree was updated with #33241 and most cases are fixed, do we still want to fix the more rare https://github.com/bitcoin-core/libmultiprocess/issues/189 for the v30 milestone?

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3241325460)
Only change is rebase.

re-ACK a4fa7b97aa73b177ea416388acfc2aba25c79e19 🚇

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: r
...

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2313212375)
seems fine to upstream this, but this shouldn't be a blocker, i'd say

💬 dergoegge commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2313210541)
```suggestion
Assume(req.indexes[i] > req.indexes[i-1]);
```

Just so the assertion isn't active in release builds

💬 dergoegge commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2313213195)
There's only one invariant we're checking, perhaps we don't need the function?

💬 dergoegge commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2313212435)
The two comments are saying the same thing, I'd suggest to just keep one

💬 maflcko commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3241343804)
ctest doesn't have a default timeout, so it would be a bit odd to expose users to a unit test run that never finishes, albeit rare?

> this issue was more artificial, happening as a result of the way the test was written.


This feature is experimental anyway, so maybe the unit test could be rewritten or removed temporarily for the 30.x release branch, if fixing it is too invasive for now?

💬 Sjors commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3241353079)
Or we could add a timeout for this specific test. I don't think we should remove it, because we want to catch unknown issues on platforms / circumstances that our CI doesn't cover.

💬 maflcko commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3241422055)
> add a timeout

Sure, but I'd say the timeout should be added in the C++ code, not in ctest, possibly with an error message explaining the known issue.

💬 hebasto commented on pull request "doc: unify `datacarriersize` warning with release notes":
(https://github.com/bitcoin/bitcoin/pull/33224#issuecomment-3241549612)
> I've done some experimenting with a test project on transifex to see what the actual effects this change would cause, and a potential workaround.
>
> In this test project, I copied over a couple fully translated languages to observe how the translated strings change when the source is updated. After rebasing this PR branch and doing `cmake --build build --target translate`, I uploaded the resulting modified `bitcoin_en.xlf` file. This resulted in 122 (~10% of strings) being marked untransla
...

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2313401875)
Could check the return code and print something to clarify this is an error, similar to the tidy error above:

```sh
echo "^^^ ⚠️ Failure generated from clang-tidy"
false
```

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2313427999)
Merged these into a single step in fdf64e55324, as there was no clear distinction between the two steps.

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2313428760)
amedned in cc1735d7771

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2313429763)
Yes I'd like to keep this for a followup for now.

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2313430468)
Done, here and elsewhere

💬 willcl-ark commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3241633101)
Rebased on master to catch upstream changes, manually added changes to align with #31802 and #33099 which were merged while I was away. Hopefully I didn't miss any others.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3241642016)
`dbd76e68c3...2bd4714fa9`: rebase due to conflicts and address https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2274730057

I am back from some afk time.

💬 hebasto commented on pull request "stabilize translations by reverting old ids by text content":
(https://github.com/bitcoin/bitcoin/pull/33270#issuecomment-3241645029)
Since we are not using [ID-based](https://doc.qt.io/qt-6/linguist-id-based-i18n.html) translations, it seems reasonable to consider removing the `id` attributes from `trans-unit` elements in the XML file.

💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3241685301)
Just some style fixups, but it looks like out-of-storage errors are happening:

```
/home/admin/actions-runner/_work/_temp/depends/work/build/arm-linux-gnueabihf/qt/6.7.3-634ec665c7f/qtbase/src/gui/util/qvalidator.h:163:2: fatal error: cannot write PCH file: No space left on device
```

re-ACK 8a7eb6ef2c226459739bdfe050b735a5a0d0b771 🕧

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisi
...