💬 fjahr commented on pull request "index: Deduplicate HashKey / HeightKey handling":
(https://github.com/bitcoin/bitcoin/pull/32997#issuecomment-3083380164)
Concept ACK on the deduplication parts

📝 maflcko opened a pull request: "ci: Run unit test parallel with functional tests"
(https://github.com/bitcoin/bitcoin/pull/33000)
Fixes https://github.com/bitcoin/bitcoin/issues/32770

📝 maflcko opened a pull request: "test: Do not pass tests on unhandled exceptions"
(https://github.com/bitcoin/bitcoin/pull/33001)
Currently the functional tests are problematic, because they pass, even if they encounter an unhanded exception.

Fix this by handling all exceptions: Catch `BaseException` as fallback and mark it as failure.

Can be tested via:

```diff
diff --git a/test/functional/wallet_disable.py b/test/functional/wallet_disable.py
index da6e5d408f..ecc41fb041 100755
--- a/test/functional/wallet_disable.py
+++ b/test/functional/wallet_disable.py
@@ -19,6 +19,7 @@ class DisableWalletTest (BitcoinTe
...

👍 hebasto approved a pull request: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32863#pullrequestreview-3028987019)
ACK 5300295083f2e199c22a7ad55e62a8dc7549a76e, I've backported all listed PRs locally (had 3 conflict to resolve), and got zero diff with this PR.

The last commit also addresses https://github.com/bitcoin/bitcoin/pull/32810#pullrequestreview-2982682093.

💬 theStack commented on pull request "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects":
(https://github.com/bitcoin/bitcoin/pull/32868#issuecomment-3083682118)
Thanks for the reviews! Rebased on master as suggested (to fix the silent merge conflict related to #32968).

💬 hebasto commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2213081726)
On Alpine Linux v3.22:
```
$ apk info -e xz | wc -l
0
$ gmake -C depends freetype_extracted # Succeeds, just like for any other package with a .tar.xz tarball.
```

🤔 hebasto reviewed a pull request: "doc: add alpine build instructions"
(https://github.com/bitcoin/bitcoin/pull/32559#pullrequestreview-3029112960)
ACK 4f502baf8f649e30d9495760b54080c272882213.

💬 b-l-u-e commented on pull request "tests: Remove hardcoded addresstype in `rpc_psbt.py`":
(https://github.com/bitcoin/bitcoin/pull/32701#issuecomment-3083705983)
Hey @Christewart, I noticed this PR was closed, and I’d love to pick it up. I see that the test suite overhaul and recalculating values for multiple address types is a pretty big task, but I’m definitely up for the challenge. If you have any concerns or important details to share before I dive in, please let me know!

💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3083708045)
update: in mempool_reorg.py: “slighty” -> “slightly” [correct spelling]

💬 maflcko commented on issue "Slow unit tests delay functional tests and leave CPU unused":
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3083727391)
> cf [#32945](https://github.com/bitcoin/bitcoin/pull/32945) maybe?

Can't hurt, but depending on the sanitizer, it is a different test that is the slowest: https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-2984264066 so the approach would have to be done for other tests as well.

Submitted https://github.com/bitcoin/bitcoin/pull/33000 for the ci in the meantime.

🤔 rkrux reviewed a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3029198213)
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542

💬 rkrux commented on pull request "wallet: Set migrated wallet name only on success":
(https://github.com/bitcoin/bitcoin/pull/32984#discussion_r2213133583)
In 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542 "test: Failed load after migrate should restore backup"

Adding a review note because it took me some time to understand why this error pops up only after migration is complete and not during the process as this particular error comes from inside `CWallet::AttachChain` that is a part of `CWallet::Create`. The reason I found is that all the `CWallet::Create` calls inside the migration process pass an empty context that doesn't contain the `chain` due
...

💬 rkrux commented on pull request "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects":
(https://github.com/bitcoin/bitcoin/pull/32868#issuecomment-3083793468)
re-ACK 5fa34951ead2eebcced919537f5e27526f61d909 modulo failing CI due to silent merge conflict.

💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3083794957)
reACK a161f058089dfe5f3aa7d3b9635f0400f0147b5d

spelling fix

💬 instagibbs commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3083796940)
> Edit: Oh nvm, the other GetSigOpCount() recurses a P2SH script.

I wouldn't mind a comment spelling out what it's doing, it is a peculiar function

💬 instagibbs commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3083835263)
reACK https://github.com/bitcoin/bitcoin/pull/32973/commits/b6d4688f77df9e31fd64e2be300f55bb8e944bd0

🤔 hebasto reviewed a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3029285837)
Concept ACK.

> we qualify for an open source discount of 50%.

> We would be dependent on Cirrus infra...

We shouldn't be [surprised](https://cirrus-ci.org/blog/2023/07/17/limiting-free-usage-of-cirrus-ci/) when Cirrus suddenly changes its modus operandi, including its advertised open-source discount or general availability.

📝 maflcko opened a pull request: "ci: Only pass documented env vars"
(https://github.com/bitcoin/bitcoin/pull/33002)
The CI currently inherits almost all env vars from the host. This was problematic in the past and causing non-determinism, e.g. the fix in commit fa12558d21aa03c22407a1458a0345d8a7ab6a4b. It is still problematic today, see e.g. https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586828644, or https://github.com/bitcoin/bitcoin/issues/32935

This fixes https://github.com/bitcoin/bitcoin/issues/32935 by only passing env vars documented in `./ci/test/00_setup_env.sh`.

Implementation-wi
...

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3083905684)
> Concept ACK.
>
> > we qualify for an open source discount of 50%.
>
> > We would be dependent on Cirrus infra...
>
> We shouldn't be [surprised](https://cirrus-ci.org/blog/2023/07/17/limiting-free-usage-of-cirrus-ci/) when Cirrus suddenly changes its modus operandi, including its advertised open-source discount or general availability.

Certainly, it is good to be wary of that. I think this is equally true for all cloud providers though.

It's my belief that if we complete this mi
...

💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3083928304)
> 2\. We could always revert back to a self-hosted solution

Agree that this may happen with any third party (including GitHub itself). If we want to switch back to the self-hosted runners, it should be as trivial as `git revert $the_merge_commit_of_this_pull`. Alternatively, switch to GHA-based self-hosted runners. Though, it would be good to create a proof-of-concept pull request to switch to self-hosted runners in GHA, or a different hosted alternative (e.g. warp-build), after this pull is
...