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

💬 m3dwards commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2175762192)
Pushed new approach of calling `getaddrinfo` twice. First pass is unchanged from original behaviour, second pass adds local IP addresses should they have not been found on the first pass.

💬 m3dwards commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1644231220)
Failing run: https://github.com/m3dwards/bitcoin/actions/runs/9354822480/job/25748533616

Looks ok to me, shows exit code 1.

For completeness, here is a run with a failing test with `CI_FAILFAST_TEST_LEAVE_DANGLING` removed: https://github.com/m3dwards/bitcoin/actions/runs/9562790625/job/26359909811. The job fails with exit code 137. The windows job behaves differently but I don't think that uses `--failfast`

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644231641)
Changed to:

```cpp
# - There are no strict requirements on the hardware. Having fewer CPU threads
# than recommended merely causes the CI script to run slower.
```

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644232676)
Oh, so only the maintainer of the fork will have this problem? Maybe there's another solution then.

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644236071)
I think this extends to all hardware. For example using a spinning disk will also be slower, regardless of your CPU speed. So being less precise is more correct here.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644256566)
The list of machines only contain recommendations for CPUs and memory. Presumably giving them less memory than recommended can lead to OOM. Giving them fewer CPUs is the only recommendation you can safely ignore (if you're not in a CI environment that kills jobs after a timeout).