💬 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

💬 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%)
```

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

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

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

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

✅ fanquake closed a pull request: "Erlay: bandwidth-efficient transaction relay protocol"
(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.

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

🚀 fanquake merged a pull request: "refactor: remove extraneous lock annotations from function definitions"
(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

💬 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

💬 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()};
```

💬 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()) {
```

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

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

💬 tdb3 commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648819742)
Initially, the thought was similar to above, if specs change, do we want to come back to this file and adjust them here, or would it be better to have a place that we can point to for minimum/recommended specs more generally or globally (since building and running tests happen for development in general rather than solely in CI).
If these specs won't change very frequently, then it's not a big risk to keep them here and update this file over time.

💬 willcl-ark commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1648820384)
Yes I agree on re-reading this that this error does not quite correctly describe the specific check being failed here, which is that the outpoints vout is not higher than the number of vouts in the input, i.e. it is by definition invalid.

I have taken your suggestion on the wording in 77ef75d3ce4bff76a0db730b57896dcf7e8f3988

👍 vasild approved a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2132431022)
ACK 1949b64afe7ecf91091f58623e75341a75996da0

Thank you!

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2182554706)
It compiles and the test run, but it's not pretty.

The conversion between `Sv2NetMsg` and `CSerializedNetMsg` / `CNetMessage` is done in https://github.com/bitcoin/bitcoin/pull/30315/commits/7d937674de425d8e2525bac9e719ac93a775ea45, which is a bit of a hack at the moment.

I also haven't put much thought into `Sv2NetMsg` ever since I took over the pull request, so perhaps this design can be improved as well.