💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644151691)
Well, what problem is this trying to solve, given that it introduces new problems?

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644154081)
An alternative would be for forks to just supply an x86 machine. The CI system should detect this itself and invoke qemu-aarch64

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644156898)
Is that new behavior? Let me test that again...

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644159505)
I guess I never tested this, because the documentation just says:

```
# - arm64: For an aarch64 machine, recommended to have 2 CPUs and 8 GB of memory.
```

If the performance difference isn't too terrible, then I can just drop the commit and mention this in the documentation.

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644160436)
You'll have to run something like `podman run --rm --privileged docker.io/multiarch/qemu-user-static --reset -p yes` and then register the type as `arm64`, but I'd say it should work.

💬 paplorinc commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644161127)
We only have 3 matching lines, so extracting them would result in slightly more complicated code.
If the reviewers insist, I don't mind doing it, see https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1643367751

💬 stickies-v commented on pull request "fix: typo in benchmark documentation":
(https://github.com/bitcoin/bitcoin/pull/30296#issuecomment-2175664755)
NACK, one-off typo fixes are not a good use of anyone's time

💬 maflcko commented on pull request "ci: remove unused bcc variable from workflow":
(https://github.com/bitcoin/bitcoin/pull/30299#issuecomment-2175665297)
lgtm ACK 518b06c4b889d71a3fdd61f8fe38d519ea5e4a1b

✅ glozow closed a pull request: "fix: typo in benchmark documentation"
(https://github.com/bitcoin/bitcoin/pull/30296)

💬 glozow commented on pull request "fix: typo in benchmark documentation":
(https://github.com/bitcoin/bitcoin/pull/30296#issuecomment-2175673833)
Thanks for your interest in contributing! However, as we have hundreds of pull requests, I am closing this to focus review on the others. (See [contributing guidelines on refactoring](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring)).

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644172953)
`qemu-aarch64` adds quite a few packages to Ubuntu, so I'd probably want to keep the `NO_ARM` option around, but document this alternative.

💬 fanquake commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2175689878)
> Maybe https://github.com/bitcoin/bitcoin/commit/a491cea18ee63e514359a5ba699e8cc159664efc could be cherry-picked here to fix the multiprocess failure https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2157739910?

That very-much looks like the correct fix.

🚀 fanquake merged a pull request: "ci: remove unused bcc variable from workflow"
(https://github.com/bitcoin/bitcoin/pull/30299)

🚀 fanquake merged a pull request: "macOS: rewrite some docs & swap `mmacosx-version-min` for `mmacos-version-min`"
(https://github.com/bitcoin/bitcoin/pull/30287)

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644183858)
I guess this involves partially reverting #28087 since it seems that stripped the `qemu-arm` call from `00_setup_env_arm.sh`.

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1644188203)
I removed the error handling here for two reasons:

Firstly it just returns an empty vector which is what I believe this function will do anyway if it fails to get address info.

Secondly, if `getaddrinfo` doesn't allow loopback only IPV6 then that is returned as an error code 1 "address family for hostname not supported" so this specific case would have to be allowed.

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644202995)
I can't parse the new sentence grammatically. Maybe just say `, and the CI script can handle slower hardware.`?

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644206247)
I don't think this has anything to do with forks. It should only depend on whether cirrus is enabled on the repository where the branch is pushed.

👍 theStack approved a pull request: "netbase: extend CreateSock() to support creating arbitrary sockets"
(https://github.com/bitcoin/bitcoin/pull/30202#pullrequestreview-2125109733)
re-ACK 1245d1388b003c46092937def7041917aecec8de

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644222617)
(comments came in out of order for me)

I don't understand Docker, Podman and/or Qemu well enough to figure this out myself. For now I'll just leave this commit.