👍 vasild approved a pull request: "[PoC] ci: Add FreeBSD GitHub Actions job"
(https://github.com/bitcoin/bitcoin/pull/30164#pullrequestreview-2132233632)
ACK a622fbe70a1842396e2670050f8dd7717d764d3c More testing on different environments is better.
I have no opinion or enough information to judge the security implications for the repository (I am not sure what github actions is exactly) or the limited CI resources or how often a given task finds problems. For any new task we don't know what problems would it find regularly and how would that compare to other tasks.
Maybe, if CI resources are really scarce, an existent CI task can be flipped
...
(https://github.com/bitcoin/bitcoin/pull/30164#pullrequestreview-2132233632)
ACK a622fbe70a1842396e2670050f8dd7717d764d3c More testing on different environments is better.
I have no opinion or enough information to judge the security implications for the repository (I am not sure what github actions is exactly) or the limited CI resources or how often a given task finds problems. For any new task we don't know what problems would it find regularly and how would that compare to other tasks.
Maybe, if CI resources are really scarce, an existent CI task can be flipped
...
💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182399338)
> I've pushed a modified branch to my personal account.
Me also a few comments up :)
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182399338)
> I've pushed a modified branch to my personal account.
Me also a few comments up :)
💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182401964)
> > I've pushed a modified branch to my personal account.
>
> Me also a few comments up :)
Oh sorry, I missed it.
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182401964)
> > I've pushed a modified branch to my personal account.
>
> Me also a few comments up :)
Oh sorry, I missed it.
💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182406521)
> > I've pushed a modified branch to my personal account.
>
> Me also a few comments up :)
Could you please to rerun that job one more time to ensure that `ccache --show-stats` will print nearly 100% hit rate?
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182406521)
> > I've pushed a modified branch to my personal account.
>
> Me also a few comments up :)
Could you please to rerun that job one more time to ensure that `ccache --show-stats` will print nearly 100% hit rate?
💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182409729)
Although I can see the cache get restored the job is no faster so I'm not sure it's working.
> Could you please rerun that job one more time to ensure that ccache --show-stats will print nearly 100% hit rate?
Will do this
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182409729)
Although I can see the cache get restored the job is no faster so I'm not sure it's working.
> Could you please rerun that job one more time to ensure that ccache --show-stats will print nearly 100% hit rate?
Will do this
💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182430012)
Re-running but the cache hit rate at the moment is low:
```shell
ccache version 4.9.1
Cacheable calls: 770 / 782 (98.47%)
Hits: 59 / 770 ( 7.66%)
Direct: 7 / 59 (11.86%)
Preprocessed: 52 / 59 (88.14%)
Misses: 711 / 770 (92.34%)
Uncacheable calls: 12 / 782 ( 1.53%)
Local storage:
Cache size (GB): 0.1 / 0.1 (102.4%)
Cleanups: 604
Hits: 59 / 770 ( 7.66%)
Misses: 711 / 770 (92.34%)
```
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182430012)
Re-running but the cache hit rate at the moment is low:
```shell
ccache version 4.9.1
Cacheable calls: 770 / 782 (98.47%)
Hits: 59 / 770 ( 7.66%)
Direct: 7 / 59 (11.86%)
Preprocessed: 52 / 59 (88.14%)
Misses: 711 / 770 (92.34%)
Uncacheable calls: 12 / 782 ( 1.53%)
Local storage:
Cache size (GB): 0.1 / 0.1 (102.4%)
Cleanups: 604
Hits: 59 / 770 ( 7.66%)
Misses: 711 / 770 (92.34%)
```
💬 fanquake commented on pull request "WIP Optimize SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#issuecomment-2182433397)
> The changes make it ~10% faster
I tested this change on an aarch64 machine with GCC 14.1.1, and the benchmark was ~ the same, or slower:
Master
```bash
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 27.32 | 36,601,
...
(https://github.com/bitcoin/bitcoin/pull/30317#issuecomment-2182433397)
> The changes make it ~10% faster
I tested this change on an aarch64 machine with GCC 14.1.1, and the benchmark was ~ the same, or slower:
Master
```bash
| ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 27.32 | 36,601,
...
💬 vasild commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182440051)
Ok, a few takeaways:
* Don't enable `-fsanitize=fuzzer` automatically by e.g. `--enable-fuzz` because that would conflict with other fuzzers.
* A better name for `--enable-fuzz` would be `--disable-all-build-targets-except-fuzz` or `--build-only-fuzz` or `--enable-fuzz-binary=yes|no|exclusive`, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182440051)
Ok, a few takeaways:
* Don't enable `-fsanitize=fuzzer` automatically by e.g. `--enable-fuzz` because that would conflict with other fuzzers.
* A better name for `--enable-fuzz` would be `--disable-all-build-targets-except-fuzz` or `--build-only-fuzz` or `--enable-fuzz-binary=yes|no|exclusive`, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.
💬 paplorinc commented on pull request "WIP Optimize SipHash":
(https://github.com/bitcoin/bitcoin/pull/30317#issuecomment-2182442479)
Thanks for checking @fanquake, I noticed that the [coverage report](https://corecheck.dev/bitcoin/bitcoin/pulls/30317) also reported the same - seems the M1 processor is doing something differently.
(https://github.com/bitcoin/bitcoin/pull/30317#issuecomment-2182442479)
Thanks for checking @fanquake, I noticed that the [coverage report](https://corecheck.dev/bitcoin/bitcoin/pulls/30317) also reported the same - seems the M1 processor is doing something differently.
💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182447512)
> Re-running but the cache hit rate at the moment is low:
The recent successive "ASan + LSan + UBSan + integer, no depends, USDT" job for the master branch on the Cirrus CI was https://cirrus-ci.com/task/6204664989876224. The merged [PR](https://github.com/bitcoin/bitcoin/pull/30291) did not touch C++ code at all. However, the hit rate is far from 100%:
```
+ bash -c 'ccache --version | head -n 1 && ccache --show-stats'
ccache version 4.9.1
Cacheable calls: 769 / 781 (98.46%)
Hits:
...
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182447512)
> Re-running but the cache hit rate at the moment is low:
The recent successive "ASan + LSan + UBSan + integer, no depends, USDT" job for the master branch on the Cirrus CI was https://cirrus-ci.com/task/6204664989876224. The merged [PR](https://github.com/bitcoin/bitcoin/pull/30291) did not touch C++ code at all. However, the hit rate is far from 100%:
```
+ bash -c 'ccache --version | head -n 1 && ccache --show-stats'
ccache version 4.9.1
Cacheable calls: 769 / 781 (98.46%)
Hits:
...
✅ fanquake closed a pull request: "Erlay: bandwidth-efficient transaction relay protocol"
(https://github.com/bitcoin/bitcoin/pull/21515)
(https://github.com/bitcoin/bitcoin/pull/21515)
💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182454380)
> * `--enable-fuzz-binary=yes|no|exclusive`, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.
Seems fine to do, if `-DBUILD_FUZZ_BINARY=ON/OFF/EXCL...` is easy to implement.
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2182454380)
> * `--enable-fuzz-binary=yes|no|exclusive`, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.
Seems fine to do, if `-DBUILD_FUZZ_BINARY=ON/OFF/EXCL...` is easy to implement.
💬 maflcko commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182459530)
The Cirrus cache is shared with pull requests, so it can not be used to compare it here. You'd have to run it locally on a fresh VM, twice.
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2182459530)
The Cirrus cache is shared with pull requests, so it can not be used to compare it here. You'd have to run it locally on a fresh VM, twice.
🚀 fanquake merged a pull request: "refactor: remove extraneous lock annotations from function definitions"
(https://github.com/bitcoin/bitcoin/pull/30316)
(https://github.com/bitcoin/bitcoin/pull/30316)
🤔 vasild reviewed a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245#pullrequestreview-2132345212)
Approach ACK a7cbc27101677ee5ff357da9595f386b03b4756d
(https://github.com/bitcoin/bitcoin/pull/30245#pullrequestreview-2132345212)
Approach ACK a7cbc27101677ee5ff357da9595f386b03b4756d
💬 vasild commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1648772483)
nit: function arguments that are passed by value need not be `const`
```suggestion
auto callgetaddrinfo = [&](int flags, bool only_add_local_addr) {
```
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1648772483)
nit: function arguments that are passed by value need not be `const`
```suggestion
auto callgetaddrinfo = [&](int flags, bool only_add_local_addr) {
```
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in
💬 vasild commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1648799369)
Simpler:
```diff
- // Convert the set to a vector
- std::vector<CNetAddr> resolved_addresses;
- resolved_addresses.reserve(resolved_addresses_set.size());
- for (auto it = resolved_addresses_set.begin(); it != resolved_addresses_set.end(); ) {
- resolved_addresses.push_back(std::move(resolved_addresses_set.extract(it++).value()));
- }
-
- return resolved_addresses;
+ return {resolved_addresses_set.begin(), resolved_addresses_set.end()};
```
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1648799369)
Simpler:
```diff
- // Convert the set to a vector
- std::vector<CNetAddr> resolved_addresses;
- resolved_addresses.reserve(resolved_addresses_set.size());
- for (auto it = resolved_addresses_set.begin(); it != resolved_addresses_set.end(); ) {
- resolved_addresses.push_back(std::move(resolved_addresses_set.extract(it++).value()));
- }
-
- return resolved_addresses;
+ return {resolved_addresses_set.begin(), resolved_addresses_set.end()};
```
💬 vasild commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1648787209)
nit, feel free to ignore if you find the current one more readable, but this is equivalent to:
```suggestion
if (!only_add_local_addr || addr.IsLocal()) {
```
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1648787209)
nit, feel free to ignore if you find the current one more readable, but this is equivalent to:
```suggestion
if (!only_add_local_addr || addr.IsLocal()) {
```
💬 vasild commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1648805702)
Are you sure that `getaddrinfo()` would return error (ie `n_err != 0`) and still set something useful in `ai_res`? That sounds strange.
I am worried that it may return an error (for whatever reason) and set `ai_res` to a bogus pointer which we later try to dereference.
Just returning from the lambda and not from `WrappedGetAddrInfo()` if `getaddrinfo()` returns an error seems reasonable to me.
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1648805702)
Are you sure that `getaddrinfo()` would return error (ie `n_err != 0`) and still set something useful in `ai_res`? That sounds strange.
I am worried that it may return an error (for whatever reason) and set `ai_res` to a bogus pointer which we later try to dereference.
Just returning from the lambda and not from `WrappedGetAddrInfo()` if `getaddrinfo()` returns an error seems reasonable to me.
💬 tdb3 commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648813882)
Good point about the `+`. The thought was "Is there a way to describe what the needs while minimizing the future changes needed to this file?" If we think pointing elsewhere (e.g. to a place that describes the needs, but is being updated for additional reasons beyond this file) is overkill, that's ok.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648813882)
Good point about the `+`. The thought was "Is there a way to describe what the needs while minimizing the future changes needed to this file?" If we think pointing elsewhere (e.g. to a place that describes the needs, but is being updated for additional reasons beyond this file) is overkill, that's ok.