💬 maflcko commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2156377203)
> Passing `--exclude "rpc_bind.py --ipv6"` excludes not only the specified test but also `rpc_bind.py --ipv4` and `rpc_bind.py --nonloopback`.
This is the existing behavior on current master and this pull request. I don't think it was ever supported. Though, I guess this means this pull request isn't needed, then.
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2156377203)
> Passing `--exclude "rpc_bind.py --ipv6"` excludes not only the specified test but also `rpc_bind.py --ipv4` and `rpc_bind.py --nonloopback`.
This is the existing behavior on current master and this pull request. I don't think it was ever supported. Though, I guess this means this pull request isn't needed, then.
💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632204359)
Not sure about exposing this. This is an expert-only setting. Anyone changing it needs to understand exactly what they are doing, so they might as well modify the `ci/test/00_setup_env_native_asan.sh` config file manually.
I'd prefer to keep the `sed` approach as-is. If you want to change it, it may be better to do in a separate pull request. This way, the focus here is kept on only moving the task from one infra to the other.
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632204359)
Not sure about exposing this. This is an expert-only setting. Anyone changing it needs to understand exactly what they are doing, so they might as well modify the `ci/test/00_setup_env_native_asan.sh` config file manually.
I'd prefer to keep the `sed` approach as-is. If you want to change it, it may be better to do in a separate pull request. This way, the focus here is kept on only moving the task from one infra to the other.
💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632204875)
In theory, only the fuzz binary compilation could be excluded, because there is a separate task for the fuzz tests under asan. But either seems fine.
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632204875)
In theory, only the fuzz binary compilation could be excluded, because there is a separate task for the fuzz tests under asan. But either seems fine.
💬 maflcko commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632205551)
Looks like this imported a broken wine 9 release candidate from `debian:trixie`?
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632205551)
Looks like this imported a broken wine 9 release candidate from `debian:trixie`?
💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1632207682)
https://github.com/bitcoin/bitcoin/blob/5cc2ae4ff287c6fef10b2e634e35353588f9ea3d/contrib/verify-binaries/verify.py#L511-L515
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1632207682)
https://github.com/bitcoin/bitcoin/blob/5cc2ae4ff287c6fef10b2e634e35353588f9ea3d/contrib/verify-binaries/verify.py#L511-L515
💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#issuecomment-2156393347)
> tACK [8135632](https://github.com/bitcoin/bitcoin/commit/8135632c33f4bad8434403b5807c48d7dba3b3d7)
>
> Still one open comment regarding specifying bitcoincore.org, but happy to reACK if that gets changed, and it's not a blocker for me.
I wrote a better error message that gives a "Did you mean: <closest_match>" when the platform typed isn't in the SHA256SUMS. Avoiding the domain issue and being significantly more helpful. LMK if you like it enough to add to this PR.
(https://github.com/bitcoin/bitcoin/pull/30147#issuecomment-2156393347)
> tACK [8135632](https://github.com/bitcoin/bitcoin/commit/8135632c33f4bad8434403b5807c48d7dba3b3d7)
>
> Still one open comment regarding specifying bitcoincore.org, but happy to reACK if that gets changed, and it's not a blocker for me.
I wrote a better error message that gives a "Did you mean: <closest_match>" when the platform typed isn't in the SHA256SUMS. Avoiding the domain issue and being significantly more helpful. LMK if you like it enough to add to this PR.
💬 maflcko commented on pull request "ci: Native Windows CI job cleanup":
(https://github.com/bitcoin/bitcoin/pull/30242#issuecomment-2156398733)
ACK 0d3ef83433805d3f367130fd5bd227a8ed5a7ccd
(https://github.com/bitcoin/bitcoin/pull/30242#issuecomment-2156398733)
ACK 0d3ef83433805d3f367130fd5bd227a8ed5a7ccd
💬 maflcko commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319)
> I'm not sure if I'm doing something wrong or if there is indeed a bug where the divergent chain is not rewound. I will continue investigating.
This sounds like a bug. Just to clarify, the active chain is stuck at height `298` and the background chain continues to sync past `399`?
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319)
> I'm not sure if I'm doing something wrong or if there is indeed a bug where the divergent chain is not rewound. I will continue investigating.
This sounds like a bug. Just to clarify, the active chain is stuck at height `298` and the background chain continues to sync past `399`?
💬 maflcko commented on issue "AssumeUTXO Mainnet Readiness Tracking":
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2156407545)
29996 doesn't seem optional, but possibly a blocker, see https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319
(https://github.com/bitcoin/bitcoin/issues/29616#issuecomment-2156407545)
29996 doesn't seem optional, but possibly a blocker, see https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1632216319
💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1632217106)
I tested this myself, and the reason seems to be that absent a `HEAD` request, a 404.html would be downloaded as the targz, which then fails the shasum check.
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1632217106)
I tested this myself, and the reason seems to be that absent a `HEAD` request, a 404.html would be downloaded as the targz, which then fails the shasum check.
💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156409463)
> Moreover, the optimization to reduce two requests into one significantly lowers the traffic load on the host,
This isn't true? There are still two requests before and after this change, so this isn't changed. Also a `HEAD` request shouldn't cause any meaningful traffic, compared to a full download.
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156409463)
> Moreover, the optimization to reduce two requests into one significantly lowers the traffic load on the host,
This isn't true? There are still two requests before and after this change, so this isn't changed. Also a `HEAD` request shouldn't cause any meaningful traffic, compared to a full download.
💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1632218405)
Does this load the full response into memory, which curl does not do? (Probably fine, but if the approach is extended to debug symbols, which are 0.5GB, this may become an issue)
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1632218405)
Does this load the full response into memory, which curl does not do? (Probably fine, but if the approach is extended to debug symbols, which are 0.5GB, this may become an issue)
💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156411645)
Also tend toward NACK here, as there are several issues here from a quick glance:
* This is more lines of code, so possibly more to maintain.
* This worsens the UX, due to lack of progress.
* Review and testing this change may not be worth it, considering the limited benefit, if any?
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156411645)
Also tend toward NACK here, as there are several issues here from a quick glance:
* This is more lines of code, so possibly more to maintain.
* This worsens the UX, due to lack of progress.
* Review and testing this change may not be worth it, considering the limited benefit, if any?
💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156419616)
> > Moreover, the optimization to reduce two requests into one significantly lowers the traffic load on the host,
>
> This isn't true? There are still two requests before and after this change, so this isn't changed. Also a `HEAD` request shouldn't cause any meaningful traffic, compared to a full download.
Twice I asked if you think it's fine, we can use one request to check the status code (if it's 404) or download (if status code isn't 404) and I got no answer. Right now the code send tw
...
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156419616)
> > Moreover, the optimization to reduce two requests into one significantly lowers the traffic load on the host,
>
> This isn't true? There are still two requests before and after this change, so this isn't changed. Also a `HEAD` request shouldn't cause any meaningful traffic, compared to a full download.
Twice I asked if you think it's fine, we can use one request to check the status code (if it's 404) or download (if status code isn't 404) and I got no answer. Right now the code send tw
...
⚠️ maflcko opened an issue: "fuzz: crypter: Abrt in __cxxabiv1::failed_throw"
(https://github.com/bitcoin/bitcoin/issues/30251)
From https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69500
In:
* https://github.com/bitcoin/bitcoin/blob/4a020ca443ba370bf41583962d16aa8551876f53/src/wallet/test/fuzz/crypter.cpp#L65
* https://github.com/bitcoin/bitcoin/blob/4a020ca443ba370bf41583962d16aa8551876f53/src/wallet/crypter.cpp#L98
I haven't looked into this in detail, but it may be allocating a large chunk of memory. Given that encryption is only used for small chunks of memory (private keys), I wonder if it makes sense
...
(https://github.com/bitcoin/bitcoin/issues/30251)
From https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=69500
In:
* https://github.com/bitcoin/bitcoin/blob/4a020ca443ba370bf41583962d16aa8551876f53/src/wallet/test/fuzz/crypter.cpp#L65
* https://github.com/bitcoin/bitcoin/blob/4a020ca443ba370bf41583962d16aa8551876f53/src/wallet/crypter.cpp#L98
I haven't looked into this in detail, but it may be allocating a large chunk of memory. Given that encryption is only used for small chunks of memory (private keys), I wonder if it makes sense
...
💬 maflcko commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1632225431)
https://github.com/bitcoin/bitcoin/issues/30251
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1632225431)
https://github.com/bitcoin/bitcoin/issues/30251
💬 maflcko commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156434898)
> Twice I asked if you think it's fine [...] and I got no answer.
As the author of a pull request, there is an expectation that you spend time to review and to test the code you have written yourself. I try to ask questions during review to motivate pull request authors to look into interesting areas to explore in the change. There are currently more than 300 open pull request and I currently don't have the time to test everything I can imagine in all of them. In this case, it is not really t
...
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2156434898)
> Twice I asked if you think it's fine [...] and I got no answer.
As the author of a pull request, there is an expectation that you spend time to review and to test the code you have written yourself. I try to ask questions during review to motivate pull request authors to look into interesting areas to explore in the change. There are currently more than 300 open pull request and I currently don't have the time to test everything I can imagine in all of them. In this case, it is not really t
...
📝 maflcko opened a pull request: "test: Remove redundant verack check"
(https://github.com/bitcoin/bitcoin/pull/30252)
Currently the sync in `connect_nodes` mentions the `version` and `verack` message types, but only checks the `verack`. Neither check is required, as the `pong` check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn't add value.
Also clarify in the comments that the goal is to check the flag `fSuccessfullyConnected` indirectly.
(https://github.com/bitcoin/bitcoin/pull/30252)
Currently the sync in `connect_nodes` mentions the `version` and `verack` message types, but only checks the `verack`. Neither check is required, as the `pong` check implies both. In case of failure, the debug log will have to be consulted anyway, so the redundant check doesn't add value.
Also clarify in the comments that the goal is to check the flag `fSuccessfullyConnected` indirectly.
💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1632236142)
Done in https://github.com/bitcoin/bitcoin/pull/30252, also mentioning `fSuccessfullyConnected`. Feel free to NACK/ACK/nit.
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1632236142)
Done in https://github.com/bitcoin/bitcoin/pull/30252, also mentioning `fSuccessfullyConnected`. Feel free to NACK/ACK/nit.
📝 maflcko opened a pull request: "refactor: performance-for-range-copy in psbt.h"
(https://github.com/bitcoin/bitcoin/pull/30253)
A copy of the partial signatures is not required before serializing them.
For reference, this was found by switching the codebase to `std::span` (not sure why it wasn't found with `Span` though):
```
./psbt.h:240:23: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
240 | for (auto sig_pair : partial_sigs) {
| ^
| const &
...
(https://github.com/bitcoin/bitcoin/pull/30253)
A copy of the partial signatures is not required before serializing them.
For reference, this was found by switching the codebase to `std::span` (not sure why it wasn't found with `Span` though):
```
./psbt.h:240:23: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
240 | for (auto sig_pair : partial_sigs) {
| ^
| const &
...