💬 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)).
(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.
(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.
(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)
(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)
(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`.
(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.
(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.`?
(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.
(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
(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.
(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.
(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`
(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.
```
(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.
(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.
(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).
(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).
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644263459)
Ah wait, now I remember, reworded:
```
# When a contributor maintains a fork of the repo, any pull request they make
# to their own fork, or to the main repository, will trigger two CI runs:
# one for the branch push and one for the pull request.
# This can be avoided by setting NO_BRANCH=1 as a custom env variable in Cirrus.
```
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1644263459)
Ah wait, now I remember, reworded:
```
# When a contributor maintains a fork of the repo, any pull request they make
# to their own fork, or to the main repository, will trigger two CI runs:
# one for the branch push and one for the pull request.
# This can be avoided by setting NO_BRANCH=1 as a custom env variable in Cirrus.
```
💬 tdb3 commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644288241)
This is a good suggestion. If the file changes again, an explanation of "70" would add value to future reviewers.
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644288241)
This is a good suggestion. If the file changes again, an explanation of "70" would add value to future reviewers.
💬 tdb3 commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644326823)
Am I missing something simple? In https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1643298389, it was mentioned that -70 should pass, but -69 should not. The test case appears to check that -70 fails with `bad-txns-oversize`. If -70 passes the size checking, it would fail for a reason other than `bad-txns-oversize`, which might be worth checking (i.e. shows that CheckTransaction is proceeding past the `bad-txns-oversize` check).
Disclaimer: I didn't check the txn byte math yet fo
...
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1644326823)
Am I missing something simple? In https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1643298389, it was mentioned that -70 should pass, but -69 should not. The test case appears to check that -70 fails with `bad-txns-oversize`. If -70 passes the size checking, it would fail for a reason other than `bad-txns-oversize`, which might be worth checking (i.e. shows that CheckTransaction is proceeding past the `bad-txns-oversize` check).
Disclaimer: I didn't check the txn byte math yet fo
...