💬 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.
💬 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.
(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
(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!
(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.
(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.
💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648838279)
> 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, then we don't need to change this line.
The file is self-contained regarding this line and there is no need to point elsewhere. "podman4.1+" is equivalent to "vanilla Ubuntu 23.04+" (forever), so there is no need to update it in the future.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648838279)
> 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, then we don't need to change this line.
The file is self-contained regarding this line and there is no need to point elsewhere. "podman4.1+" is equivalent to "vanilla Ubuntu 23.04+" (forever), so there is no need to update it in the future.
✅ 0xB10C closed a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832)
(https://github.com/bitcoin/bitcoin/pull/25832)
💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648840963)
This file `.cirrus.yml` is self-contained and only concerns Cirrus CI. It has no meaning for the outside CI system. The labels are used in this config file only, they are only required here, and they are explained here.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648840963)
This file `.cirrus.yml` is self-contained and only concerns Cirrus CI. It has no meaning for the outside CI system. The labels are used in this config file only, they are only required here, and they are explained here.
💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1648841695)
The comment says 13.2+ but the code says 1400000
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1648841695)
The comment says 13.2+ but the code says 1400000
📝 0xB10C reopened a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832)
This adds five new tracepoints with documentation and tests for network connections:
- established connections with `net:inbound_connection` and `net:outbound_connection`
- closed connections (both closed by us or by the peer) with `net:closed_connnection`
- inbound connections that we choose to evict with `net:evicted_inbound_connection`
- connections that are misbehaving and punished with `net:misbehaving_connection`
I've been using these tracepoints for a few months now to monitor co
...
(https://github.com/bitcoin/bitcoin/pull/25832)
This adds five new tracepoints with documentation and tests for network connections:
- established connections with `net:inbound_connection` and `net:outbound_connection`
- closed connections (both closed by us or by the peer) with `net:closed_connnection`
- inbound connections that we choose to evict with `net:evicted_inbound_connection`
- connections that are misbehaving and punished with `net:misbehaving_connection`
I've been using these tracepoints for a few months now to monitor co
...
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2182585453)
Must have closed this while force pushing. That was not my intention.
I've rebased this with https://github.com/bitcoin/bitcoin/pull/29575 merged. The `net:misbehaving_connection` changed from
```c++
TRACE5(net, misbehaving_connection,
peer.m_id,
score_before,
howmuch,
message.c_str(),
discourage_threshold_exceeded);
```
to
```c++
TRACE2(net, misbehaving_connection,
peer.m_id,
message.c_str());
```
I've updates the tests and the bpftrace demo script acco
...
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2182585453)
Must have closed this while force pushing. That was not my intention.
I've rebased this with https://github.com/bitcoin/bitcoin/pull/29575 merged. The `net:misbehaving_connection` changed from
```c++
TRACE5(net, misbehaving_connection,
peer.m_id,
score_before,
howmuch,
message.c_str(),
discourage_threshold_exceeded);
```
to
```c++
TRACE2(net, misbehaving_connection,
peer.m_id,
message.c_str());
```
I've updates the tests and the bpftrace demo script acco
...